Merge pull request #263 from dwnusbaum/robust-action-deserialization

Use lists instead of arrays for `FlowNode` actions to make deserialization more robust
jenkinsci · May 1, 2024 · 6713aed · 6713aed
2 parents 5013153 + d682666
commit 6713aed
Show file tree

Hide file tree

Showing 26 changed files with 473 additions and 12 deletions.
diff --git a/src/main/java/org/jenkinsci/plugins/workflow/support/storage/BulkFlowNodeStorage.java b/src/main/java/org/jenkinsci/plugins/workflow/support/storage/BulkFlowNodeStorage.java
@@ -41,6 +41,7 @@
 import java.lang.reflect.Field;
 import java.lang.reflect.InvocationTargetException;
 import java.lang.reflect.Method;
+import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashMap;
@@ -138,7 +139,7 @@ public void storeNode(@NonNull FlowNode n, boolean delayWritingActions) throws I
         if (t != null) {
             t.node = n;
             List<Action> act = n.getActions();
-            t.actions = act.toArray(new Action[act.size()]);
+            t.actions = new ArrayList<>(act);
         } else {
             getOrLoadNodes().put(n.getId(), new Tag(n, n.getActions()));
         }
@@ -203,7 +204,7 @@ public void saveActions(@NonNull FlowNode node, @NonNull List<Action> actions) t
         if (t != null) {
             t.node = node;
             List<Action> act = node.getActions();
-            t.actions = act.toArray(new Action[act.size()]);
+            t.actions = new ArrayList<>(act);
         } else {
             map.put(node.getId(), new Tag(node, actions));
         }
@@ -222,11 +223,11 @@ public boolean isPersistedFully() {
      */
     private static class Tag {
         /* @NonNull except perhaps after deserialization */ FlowNode node;
-        private @CheckForNull Action[] actions;
+        private @CheckForNull List<Action> actions;
 
         private Tag(@NonNull FlowNode node, @NonNull List<Action> actions) {
             this.node = node;
-            this.actions = actions.isEmpty() ? null : actions.toArray(new Action[actions.size()]);
+            this.actions = actions.isEmpty() ? null : new ArrayList<>(actions);
         }
 
         private void storeActions() {  // We've already loaded the actions, may as well store them to the FlowNode
@@ -238,7 +239,7 @@ private void storeActions() {  // We've already loaded the actions, may as well
         }
 
         public @NonNull List<Action> actions() {
-            return actions != null ? Arrays.asList(actions) : Collections.emptyList();
+            return actions != null ? Collections.unmodifiableList(actions) : Collections.emptyList();
         }
     }
 

diff --git a/...ain/java/org/jenkinsci/plugins/workflow/support/storage/SimpleXStreamFlowNodeStorage.java b/...ain/java/org/jenkinsci/plugins/workflow/support/storage/SimpleXStreamFlowNodeStorage.java
@@ -226,11 +226,11 @@ private Tag load(String id) throws IOException {
      */
     private static class Tag {
         final /* @NonNull except perhaps after deserialization */ FlowNode node;
-        private final @CheckForNull Action[] actions;
+        private final @CheckForNull List<Action> actions;
 
         private Tag(@NonNull FlowNode node, @NonNull List<Action> actions) {
             this.node = node;
-            this.actions = actions.isEmpty() ? null : actions.toArray(new Action[actions.size()]);
+            this.actions = actions.isEmpty() ? null : new ArrayList<>(actions);
         }
 
         private void storeActions() {  // We've already loaded the actions, may as well store them to the FlowNode
@@ -242,7 +242,7 @@ private void storeActions() {  // We've already loaded the actions, may as well
         }
 
         public @NonNull List<Action> actions() {
-            return actions != null ? Arrays.asList(actions) : Collections.emptyList();
+            return actions != null ? Collections.unmodifiableList(actions) : Collections.emptyList();
         }
     }
 

diff --git a/src/test/java/org/jenkinsci/plugins/workflow/support/storage/BulkFlowNodeStorageTest.java b/src/test/java/org/jenkinsci/plugins/workflow/support/storage/BulkFlowNodeStorageTest.java
@@ -24,24 +24,35 @@
 
 package org.jenkinsci.plugins.workflow.support.storage;
 
+import hudson.model.Result;
+import hudson.util.RobustReflectionConverter;
 import java.io.File;
 import java.util.ArrayList;
+import java.util.logging.Level;
 import java.util.stream.Collectors;
 import java.util.stream.IntStream;
 import javax.xml.parsers.DocumentBuilderFactory;
-import static org.hamcrest.MatcherAssert.assertThat;
-import static org.hamcrest.Matchers.is;
+import org.jenkinsci.plugins.workflow.actions.LabelAction;
 import org.jenkinsci.plugins.workflow.cps.CpsFlowDefinition;
+import org.jenkinsci.plugins.workflow.cps.nodes.StepStartNode;
 import org.jenkinsci.plugins.workflow.job.WorkflowJob;
 import org.junit.Rule;
 import org.junit.Test;
 import org.jvnet.hudson.test.JenkinsRule;
+import org.jvnet.hudson.test.LoggerRule;
+import org.jvnet.hudson.test.recipes.LocalData;
 import org.w3c.dom.Element;
 import org.w3c.dom.Node;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.not;
+import static org.hamcrest.Matchers.nullValue;
 
 public final class BulkFlowNodeStorageTest {
 
     @Rule public JenkinsRule r = new JenkinsRule();
+    @Rule public LoggerRule logger = new LoggerRule().record(RobustReflectionConverter.class, Level.FINE).capture(50);
 
     @Test public void orderOfEntries() throws Exception {
         var p = r.createProject(WorkflowJob.class, "p");
@@ -58,4 +69,40 @@ public final class BulkFlowNodeStorageTest {
         assertThat(ids, is(IntStream.rangeClosed(2, 43).mapToObj(Integer::toString).collect(Collectors.toList())));
     }
 
+    @LocalData
+    @Test public void actionDeserializationShouldBeRobust() throws Exception {
+        /*
+        var p = r.createProject(WorkflowJob.class);
+        p.addProperty(new DurabilityHintJobProperty(FlowDurabilityHint.PERFORMANCE_OPTIMIZED));
+        p.setDefinition(new CpsFlowDefinition(
+                "stage('test') {\n" +
+                "  sleep 120\n" +
+                "}\n", true));
+        var b = p.scheduleBuild2(0).waitForStart();
+        Thread.sleep(5*1000);
+        ((CpsFlowExecution) b.getExecution()).getStorage().flush();
+        */
+        var p = r.jenkins.getItemByFullName("test0", WorkflowJob.class);
+        var b = p.getLastBuild();
+        // Build is unresumable because the local data was created with PERFORMANCE_OPTIMIZED without a clean shutdown.
+        r.assertBuildStatus(Result.FAILURE, r.waitForCompletion(b));
+        // Existing flow nodes should still be preserved though.
+        var stageBodyStartNode = (StepStartNode) b.getExecution().getNode("4");
+        assertThat(stageBodyStartNode, not(nullValue()));
+        var label = stageBodyStartNode.getPersistentAction(LabelAction.class);
+        assertThat(label.getDisplayName(), equalTo("test"));
+    }
+    // Used to create @LocalData for above test:
+    /*
+    public static class MyAction extends InvisibleAction implements PersistentAction {
+        private final String value = "42";
+    }
+    @TestExtension("actionDeserializationShouldBeRobust")
+    public static class MyActionAdder implements GraphListener.Synchronous {
+        @Override
+        public void onNewHead(FlowNode node) {
+            node.addAction(new MyAction());
+        }
+    }
+    */
 }
diff --git a/src/test/java/org/jenkinsci/plugins/workflow/support/storage/SimpleXStreamStorageTest.java b/src/test/java/org/jenkinsci/plugins/workflow/support/storage/SimpleXStreamStorageTest.java
@@ -1,20 +1,33 @@
 package org.jenkinsci.plugins.workflow.support.storage;
 
 
+import hudson.model.Result;
+import hudson.util.RobustReflectionConverter;
+import java.io.File;
+import java.util.logging.Level;
 import org.jenkinsci.plugins.workflow.actions.BodyInvocationAction;
 import org.jenkinsci.plugins.workflow.actions.LabelAction;
 import org.jenkinsci.plugins.workflow.actions.TimingAction;
+import org.jenkinsci.plugins.workflow.cps.nodes.StepStartNode;
 import org.jenkinsci.plugins.workflow.graph.AtomNode;
+import org.jenkinsci.plugins.workflow.job.WorkflowJob;
 import org.junit.Assert;
+import org.junit.Rule;
 import org.junit.Test;
-
-import java.io.File;
+import org.jvnet.hudson.test.LoggerRule;
+import org.jvnet.hudson.test.recipes.LocalData;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.not;
+import static org.hamcrest.Matchers.nullValue;
 
 /**
  * Tries to test the storage engine
  */
 public class SimpleXStreamStorageTest extends AbstractStorageTest {
 
+    @Rule public LoggerRule logger = new LoggerRule().record(RobustReflectionConverter.class, Level.FINE).capture(50);
+
     @Override
     public FlowNodeStorage instantiateStorage(MockFlowExecution exec, File storageDirectory) {
         return new SimpleXStreamFlowNodeStorage(exec, storageDirectory);
@@ -72,4 +85,39 @@ public void testDeferWriteAndFlush() throws Exception {
         mock2.setStorage(storageAfterRead);
         StorageTestUtils.assertNodesMatch(deferredWriteNode, storageAfterRead.getNode(deferredWriteNode.getId()));
     }
+
+    @LocalData
+    @Test public void actionDeserializationShouldBeRobust() throws Exception {
+        /*
+        var p = j.createProject(WorkflowJob.class);
+        p.addProperty(new DurabilityHintJobProperty(FlowDurabilityHint.MAX_SURVIVABILITY));
+        p.setDefinition(new CpsFlowDefinition(
+                "stage('test') {\n" +
+                "  sleep 120\n" +
+                "}\n", true));
+        var b = p.scheduleBuild2(0).waitForStart();
+        Thread.sleep(5*1000);
+        ((CpsFlowExecution) b.getExecution()).getStorage().flush();
+        */
+        var p = j.jenkins.getItemByFullName("test0", WorkflowJob.class);
+        var b = p.getLastBuild();
+        j.assertBuildStatus(Result.SUCCESS, j.waitForCompletion(b));
+        var stageBodyStartNode = (StepStartNode) b.getExecution().getNode("4");
+        assertThat(stageBodyStartNode, not(nullValue()));
+        var label = stageBodyStartNode.getPersistentAction(LabelAction.class);
+        assertThat(label.getDisplayName(), equalTo("test"));
+    }
+    // Used to create @LocalData for above test:
+    /*
+    public static class MyAction extends InvisibleAction implements PersistentAction {
+        private final String value = "42";
+    }
+    @TestExtension("actionDeserializationShouldBeRobust")
+    public static class MyActionAdder implements GraphListener.Synchronous {
+        @Override
+        public void onNewHead(FlowNode node) {
+            node.addAction(new MyAction());
+        }
+    }
+    */
 }
diff --git a/...BulkFlowNodeStorageTest/actionDeserializationShouldBeRobust/jobs/test0/builds/1/build.xml b/...BulkFlowNodeStorageTest/actionDeserializationShouldBeRobust/jobs/test0/builds/1/build.xml
@@ -0,0 +1,46 @@
+<?xml version="1.1" encoding="UTF-8"?>
+<flow-build plugin="workflow-job@1326.ve643e00e9220">
+  <actions/>
+  <queueId>1</queueId>
+  <timestamp>1714512454761</timestamp>
+  <startTime>1714512454774</startTime>
+  <duration>0</duration>
+  <charset>UTF-8</charset>
+  <keepLog>false</keepLog>
+  <execution class="org.jenkinsci.plugins.workflow.cps.CpsFlowExecution">
+    <result>SUCCESS</result>
+    <script>stage(&apos;test&apos;) {
+  sleep 120
+}
+</script>
+    <loadedScripts class="map"/>
+    <durabilityHint>PERFORMANCE_OPTIMIZED</durabilityHint>
+    <timings class="map">
+      <entry>
+        <string>flowNode</string>
+        <long>3116376</long>
+      </entry>
+      <entry>
+        <string>classLoad</string>
+        <long>125882830</long>
+      </entry>
+      <entry>
+        <string>run</string>
+        <long>80228333</long>
+      </entry>
+      <entry>
+        <string>parse</string>
+        <long>213222375</long>
+      </entry>
+    </timings>
+    <internalCalls class="sorted-set"/>
+    <sandbox>true</sandbox>
+    <iota>5</iota>
+    <head>1:5</head>
+    <start>2</start>
+    <done>false</done>
+    <resumeBlocked>false</resumeBlocked>
+  </execution>
+  <completed>false</completed>
+  <checkouts class="hudson.util.PersistedList"/>
+</flow-build>
diff --git a/...orage/BulkFlowNodeStorageTest/actionDeserializationShouldBeRobust/jobs/test0/builds/1/log b/...orage/BulkFlowNodeStorageTest/actionDeserializationShouldBeRobust/jobs/test0/builds/1/log
@@ -0,0 +1,6 @@
+Started
+[8mha:////4Er24oWfBBiEN/Wa7id5o9M/o5Uqubl8YTcDNNanS4ZDAAAAoh+LCAAAAAAAAP9tjTEOwjAQBM8BClpKHuFItIiK1krDC0x8GCfWnbEdkooX8TX+gCESFVvtrLSa5wtWKcKBo5UdUu8otU4GP9jS5Mixv3geZcdn2TIl9igbHBs2eJyx4YwwR1SwULBGaj0nRzbDRnX6rmuvydanHMu2V1A5c4MHCFXMWcf8hSnC9jqYxPTz/BXAFEIGsfuclm8zQVqFvQAAAA==[0m[Pipeline] Start of Pipeline
+[8mha:////4P/BX1JLzbsCrvx6yk49claR+ti0ovHPJGy4iulK196/AAAApR+LCAAAAAAAAP9tjTEOwjAUQ3+KOrAycohUghExsUZZOEFIQkgb/d8mKe3EibgadyBQiQlLlmxL1nu+oE4RjhQdby12HpP2vA+jK4lPFLtroIm3dOGaMFGwXNpJkrGnpUrKFhaxClYC1hZ1oOTRZdiIVt1VExS65pxj2Q4CKm8GeAAThZxVzN8yR9jeRpMIf5y/AJj7DGxXvP/86jduZBmjwAAAAA==[0m[Pipeline] stage
+[8mha:////4JdiHwYzioWlo2ROxl2Zlo74y53NN96hzyE6iT+69Eh0AAAApR+LCAAAAAAAAP9tjTEOwjAUQ3+KOrAycoh0gA0xsUZZOEFIQkgb/d8mKe3EibgadyBQiQlLlmxL1nu+oE4RjhQdby12HpP2vA+jK4lPFLtroIm3dOGaMFGwXNpJkrGnpUrKFhaxClYC1hZ1oOTRZdiIVt1VExS65pxj2Q4CKm8GeAAThZxVzN8yR9jeRpMIf5y/AJj7DGxXvP/86jfoP95RwAAAAA==[0m[Pipeline] { (test)
+[8mha:////4IbbNcCvGzKmOkFNSNkP4IRFXNaMMv4VDyWQd8mj5yT1AAAAoh+LCAAAAAAAAP9tjTEOAiEURD9rLGwtPQTbaGWsbAmNJ0AWEZb8zwLrbuWJvJp3kLiJlZNMMm+a93rDOic4UbLcG+wdZu14DKOti0+U+lugiXu6ck2YKRguzSSpM+cFJRUDS1gDKwEbgzpQdmgLbIVXD9UGhba9lFS/o4DGdQM8gYlqLiqVL8wJdvexy4Q/z18BzLEA29ce4gdpL1fxvAAAAA==[0m[Pipeline] sleep
+Sleeping for 2 min 0 sec
diff --git a/...BulkFlowNodeStorageTest/actionDeserializationShouldBeRobust/jobs/test0/builds/1/log-index b/...BulkFlowNodeStorageTest/actionDeserializationShouldBeRobust/jobs/test0/builds/1/log-index
@@ -0,0 +1 @@
+1231 5
diff --git a/...geTest/actionDeserializationShouldBeRobust/jobs/test0/builds/1/workflow/flowNodeStore.xml b/...geTest/actionDeserializationShouldBeRobust/jobs/test0/builds/1/workflow/flowNodeStore.xml
@@ -0,0 +1,106 @@
+<?xml version="1.1" encoding="UTF-8"?>
+<linked-hash-map>
+  <entry>
+    <string>2</string>
+    <Tag>
+      <node class="org.jenkinsci.plugins.workflow.graph.FlowStartNode" plugin="workflow-api@1283.v99c10937efcb_">
+        <parentIds/>
+        <id>2</id>
+      </node>
+      <actions>
+        <org.jenkinsci.plugins.workflow.support.storage.BulkFlowNodeStorageTest_-MyAction>
+          <value>42</value>
+        </org.jenkinsci.plugins.workflow.support.storage.BulkFlowNodeStorageTest_-MyAction>
+        <wf.a.TimingAction plugin="workflow-api@1283.v99c10937efcb_">
+          <startTime>1714512455074</startTime>
+        </wf.a.TimingAction>
+      </actions>
+    </Tag>
+  </entry>
+  <entry>
+    <string>3</string>
+    <Tag>
+      <node class="cps.n.StepStartNode" plugin="workflow-cps@3806.va_3a_6988277b_2">
+        <parentIds>
+          <string>2</string>
+        </parentIds>
+        <id>3</id>
+        <descriptorId>org.jenkinsci.plugins.workflow.support.steps.StageStep</descriptorId>
+      </node>
+      <actions>
+        <s.a.LogStorageAction/>
+        <cps.a.ArgumentsActionImpl plugin="workflow-cps@3806.va_3a_6988277b_2">
+          <arguments>
+            <entry>
+              <string>name</string>
+              <string>test</string>
+            </entry>
+          </arguments>
+          <sensitiveVariables/>
+          <isUnmodifiedBySanitization>true</isUnmodifiedBySanitization>
+        </cps.a.ArgumentsActionImpl>
+        <wf.a.TimingAction plugin="workflow-api@1283.v99c10937efcb_">
+          <startTime>1714512455144</startTime>
+        </wf.a.TimingAction>
+        <org.jenkinsci.plugins.workflow.support.storage.BulkFlowNodeStorageTest_-MyAction>
+          <value>42</value>
+        </org.jenkinsci.plugins.workflow.support.storage.BulkFlowNodeStorageTest_-MyAction>
+      </actions>
+    </Tag>
+  </entry>
+  <entry>
+    <string>4</string>
+    <Tag>
+      <node class="cps.n.StepStartNode" plugin="workflow-cps@3806.va_3a_6988277b_2">
+        <parentIds>
+          <string>3</string>
+        </parentIds>
+        <id>4</id>
+        <descriptorId>org.jenkinsci.plugins.workflow.support.steps.StageStep</descriptorId>
+      </node>
+      <actions>
+        <wf.a.BodyInvocationAction plugin="workflow-api@1283.v99c10937efcb_"/>
+        <wf.a.LabelAction plugin="workflow-api@1283.v99c10937efcb_">
+          <displayName>test</displayName>
+        </wf.a.LabelAction>
+        <wf.a.TimingAction plugin="workflow-api@1283.v99c10937efcb_">
+          <startTime>1714512455148</startTime>
+        </wf.a.TimingAction>
+        <org.jenkinsci.plugins.workflow.support.storage.BulkFlowNodeStorageTest_-MyAction>
+          <value>42</value>
+        </org.jenkinsci.plugins.workflow.support.storage.BulkFlowNodeStorageTest_-MyAction>
+      </actions>
+    </Tag>
+  </entry>
+  <entry>
+    <string>5</string>
+    <Tag>
+      <node class="cps.n.StepAtomNode" plugin="workflow-cps@3806.va_3a_6988277b_2">
+        <parentIds>
+          <string>4</string>
+        </parentIds>
+        <id>5</id>
+        <descriptorId>org.jenkinsci.plugins.workflow.steps.SleepStep</descriptorId>
+      </node>
+      <actions>
+        <cps.a.ArgumentsActionImpl plugin="workflow-cps@3806.va_3a_6988277b_2">
+          <arguments>
+            <entry>
+              <string>time</string>
+              <long>120</long>
+            </entry>
+          </arguments>
+          <sensitiveVariables/>
+          <isUnmodifiedBySanitization>true</isUnmodifiedBySanitization>
+        </cps.a.ArgumentsActionImpl>
+        <wf.a.TimingAction plugin="workflow-api@1283.v99c10937efcb_">
+          <startTime>1714512455155</startTime>
+        </wf.a.TimingAction>
+        <org.jenkinsci.plugins.workflow.support.storage.BulkFlowNodeStorageTest_-MyAction>
+          <value>42</value>
+        </org.jenkinsci.plugins.workflow.support.storage.BulkFlowNodeStorageTest_-MyAction>
+        <s.a.LogStorageAction/>
+      </actions>
+    </Tag>
+  </entry>
+</linked-hash-map>
diff --git a/...e/BulkFlowNodeStorageTest/actionDeserializationShouldBeRobust/jobs/test0/builds/legacyIds b/...e/BulkFlowNodeStorageTest/actionDeserializationShouldBeRobust/jobs/test0/builds/legacyIds
diff --git a/.../BulkFlowNodeStorageTest/actionDeserializationShouldBeRobust/jobs/test0/builds/permalinks b/.../BulkFlowNodeStorageTest/actionDeserializationShouldBeRobust/jobs/test0/builds/permalinks
@@ -0,0 +1 @@
+lastSuccessfulBuild -1