From a7a926e23d6153a66a4eaf28bfc7fd2cc9413398 Mon Sep 17 00:00:00 2001 From: Sam Van Oort Date: Thu, 15 Mar 2018 22:41:58 -0400 Subject: [PATCH 01/25] Enable persistence of the done field to provide protection against mismatches in the final FlowNode --- .../org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java b/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java index eeb511658..d7999a845 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java @@ -1523,6 +1523,7 @@ public void marshal(Object source, HierarchicalStreamWriter w, MarshallingContex for (BlockStartNode st : e.startNodes) { writeChild(w, context, "start", st.getId(), String.class); } + writeChild(w, context, "done", e.done, Boolean.class); } writeChild(w, context, "resumeBlocked", e.resumeBlocked, Boolean.class); @@ -1587,6 +1588,9 @@ public Object unmarshal(HierarchicalStreamReader reader, final UnmarshallingCont } else if (nodeName.equals("iota")) { Integer iota = readChild(reader, context, Integer.class, result); setField(result, "iota", new AtomicInteger(iota)); + } else if (nodeName.equals("done")) { + Boolean isDone = readChild(reader, context, Boolean.class, result); + setField(result, "done", isDone.booleanValue()); } else if (nodeName.equals("start")) { String id = readChild(reader, context, String.class, result); result.startNodesSerial.add(id); From 7b2ba8663bd63b9ed4da784b61d6b3eb7f9ae8a6 Mon Sep 17 00:00:00 2001 From: Sam Van Oort Date: Fri, 16 Mar 2018 17:25:55 -0400 Subject: [PATCH 02/25] Hardening and logging for abnormal cases with onLoad and incomplete persistence --- .../workflow/cps/CpsFlowExecution.java | 49 +++++++++++++++---- .../plugins/workflow/cps/FlowHead.java | 5 ++ 2 files changed, 45 insertions(+), 9 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java b/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java index d7999a845..ffbd23632 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java @@ -134,6 +134,8 @@ import java.util.Set; import java.util.concurrent.ConcurrentMap; import java.util.concurrent.TimeUnit; +import java.util.stream.Collector; +import java.util.stream.Collectors; import javax.annotation.CheckForNull; import javax.annotation.Nonnull; import javax.annotation.concurrent.GuardedBy; @@ -313,7 +315,7 @@ public class CpsFlowExecution extends FlowExecution implements BlockableResume { * {@link FlowExecution} gets loaded into memory for the build records that have been completed, * and for those we don't want to load the program state, so that check should be efficient. */ - private boolean done; + private Boolean done; /** * Groovy compiler with CPS+sandbox transformation correctly setup. @@ -598,14 +600,35 @@ int approximateNodeCount() { return iota.get(); } + /** For diagnostic purposes only. */ + private String getHeadsAsString() { + NavigableMap myHeads = this.heads; + if (myHeads == null) { + return "null-heads"; + } else if (myHeads.size() == 0) { + return "empty-heads"; + } else { + return myHeads.entrySet().stream().map(h->h.getKey()+"::"+h.getValue()).collect(Collectors.joining(",")); + } + + } + /** Handle failures where we can't load heads. */ private void rebuildEmptyGraph() { synchronized (this) { // something went catastrophically wrong and there's no live head. fake one + LOGGER.log(Level.WARNING, "Failed to load pipeline heads, so faking some up for execution " + this.toString()); if (this.startNodes == null) { this.startNodes = new Stack(); } - this.heads.clear(); + + if (this.heads != null && this.heads.size() > 0) { + if (LOGGER.isLoggable(Level.WARNING)) { + LOGGER.log(Level.WARNING, "Resetting heads to rebuild the Pipeline structure, tossing existing heads: "+getHeadsAsString()); + } + this.heads.clear(); + } + this.startNodes.clear(); FlowHead head = new FlowHead(this); heads.put(head.getId(), head); @@ -696,13 +719,15 @@ public void onLoad(FlowExecutionOwner owner) throws IOException { if (canResume()) { loadProgramAsync(getProgramDataFile()); } else { - // TODO if possible, consider tyring to close out unterminated blocks to keep existing graph history + // TODO if possible, consider trying to close out unterminated blocks to keep existing graph history // That way we can visualize the graph in some error cases. LOGGER.log(Level.WARNING, "Pipeline state not properly persisted, cannot resume "+owner.getUrl()); throw new IOException("Cannot resume build -- was not cleanly saved when Jenkins shut down."); } + } else if (done == Boolean.TRUE && !super.isComplete()) { + LOGGER.log(Level.WARNING, "Completed flow without FlowEndNode: "+this+" heads:"+getHeadsAsString()); } - } catch (IOException e) { + } catch (Exception e) { // Multicatch ensures that failure to load does not nuke the master SettableFuture p = SettableFuture.create(); programPromise = p; loadProgramFailed(e, p); @@ -829,6 +854,11 @@ void croak(Throwable t) { setResult(Result.FAILURE); onProgramEnd(new Outcome(null, t)); cleanUpHeap(); + try { + saveOwner(); + } catch (Exception ex) { + LOGGER.log(Level.WARNING, "Failed to persist WorkflowRun after noting a serious failure for run:", owner); + } } /** @@ -1156,7 +1186,7 @@ public static void maybeAutoPersistNode(@Nonnull FlowNode node) { @Override public boolean isComplete() { - return done || super.isComplete(); + return done == Boolean.TRUE || super.isComplete(); // Compare to Boolean.TRUE so null == false. } /** @@ -1183,7 +1213,6 @@ synchronized void onProgramEnd(Outcome outcome) { } catch (IOException ioe) { LOGGER.log(Level.WARNING, "Error flushing FlowNodeStorage to disk at end of run", ioe); } - } void cleanUpHeap() { @@ -1523,7 +1552,9 @@ public void marshal(Object source, HierarchicalStreamWriter w, MarshallingContex for (BlockStartNode st : e.startNodes) { writeChild(w, context, "start", st.getId(), String.class); } - writeChild(w, context, "done", e.done, Boolean.class); + if (e.done != null) { + writeChild(w, context, "done", e.done, Boolean.class); + } } writeChild(w, context, "resumeBlocked", e.resumeBlocked, Boolean.class); @@ -1590,7 +1621,7 @@ public Object unmarshal(HierarchicalStreamReader reader, final UnmarshallingCont setField(result, "iota", new AtomicInteger(iota)); } else if (nodeName.equals("done")) { Boolean isDone = readChild(reader, context, Boolean.class, result); - setField(result, "done", isDone.booleanValue()); + setField(result, "done", isDone); } else if (nodeName.equals("start")) { String id = readChild(reader, context, String.class, result); result.startNodesSerial.add(id); @@ -1754,7 +1785,7 @@ public void autopersist(@Nonnull FlowNode n) throws IOException { /** Save the owner that holds this execution. */ void saveOwner() { try { - if (this.owner.getExecutable() instanceof Saveable) { + if (this.owner != null && this.owner.getExecutable() instanceof Saveable) { // Null-check covers some anomalous cases we've seen Saveable saveable = (Saveable)(this.owner.getExecutable()); saveable.save(); } diff --git a/src/main/java/org/jenkinsci/plugins/workflow/cps/FlowHead.java b/src/main/java/org/jenkinsci/plugins/workflow/cps/FlowHead.java index bdf825770..742e0281d 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/cps/FlowHead.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/cps/FlowHead.java @@ -181,4 +181,9 @@ private Object readResolve() { private static final Logger LOGGER = Logger.getLogger(FlowHead.class.getName()); private static final long serialVersionUID = 1L; + + @Override + public String toString() { + return id+":"+head; + } } From c0286820f7aace3a16b17c09a05765afa07d7cc1 Mon Sep 17 00:00:00 2001 From: Sam Van Oort Date: Fri, 16 Mar 2018 17:48:27 -0400 Subject: [PATCH 03/25] Findbugs fixes --- .../org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java b/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java index ffbd23632..d9e54b702 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java @@ -601,7 +601,7 @@ int approximateNodeCount() { } /** For diagnostic purposes only. */ - private String getHeadsAsString() { + private synchronized String getHeadsAsString() { NavigableMap myHeads = this.heads; if (myHeads == null) { return "null-heads"; @@ -710,6 +710,7 @@ public boolean canResume() { } @Override + @SuppressFBWarnings(value = "RC_REF_COMPARISON_BAD_PRACTICE_BOOLEAN", justification = "We want to explicitly check for boolean not-null and true") public void onLoad(FlowExecutionOwner owner) throws IOException { this.owner = owner; try { @@ -1185,6 +1186,7 @@ public static void maybeAutoPersistNode(@Nonnull FlowNode node) { } @Override + @SuppressFBWarnings(value = "RC_REF_COMPARISON_BAD_PRACTICE_BOOLEAN", justification = "We want to explicitly check for boolean not-null and true") public boolean isComplete() { return done == Boolean.TRUE || super.isComplete(); // Compare to Boolean.TRUE so null == false. } From 046e27afebf1d7e8dfbaaa92a638f77874b3bb5d Mon Sep 17 00:00:00 2001 From: Sam Van Oort Date: Fri, 16 Mar 2018 21:09:48 -0400 Subject: [PATCH 04/25] More logging and done flags --- .../plugins/workflow/cps/CpsFlowExecution.java | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java b/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java index d9e54b702..9afc91545 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java @@ -660,6 +660,7 @@ protected synchronized void initializeStorage() throws IOException { h.setForDeserialize(storage.getNode(entry.getValue())); heads.put(h.getId(), h); } else { + LOGGER.log(Level.WARNING, "Tried to load head FlowNodes for execution "+this.owner+" but FlowNode was not found in storage for head id:FlowNodeId "+entry.getKey()+":"+entry.getValue()); storageErrors = true; break; } @@ -677,6 +678,7 @@ protected synchronized void initializeStorage() throws IOException { startNodes.add((BlockStartNode) storage.getNode(id)); } else { // TODO if possible, consider trying to close out unterminated blocks using heads, to keep existing graph history + LOGGER.log(Level.WARNING, "Tried to load startNode FlowNodes for execution "+this.owner+" but FlowNode was not found in storage for FlowNode Id "+id); storageErrors = true; break; } @@ -817,6 +819,7 @@ private void loadProgramFailed(final Throwable problem, SettableFuture 1) { + LOGGER.log(Level.WARNING, "Execution "+this.owner+" finished but had more than one final FlowNode where one expected, heads are: "+this.getHeadsAsString()); + } + + String tempIotaStr = Integer.toString(this.iota.get()); + FlowHead lastHead = heads.get(tempIotaStr); + if (lastHead == null || lastHead.get() == null || !(lastHead.get().getId().equals(tempIotaStr))) { + LOGGER.log(Level.WARNING, "Invalid final head for execution "+this.owner+" with head: "+lastHead); + } } try { @@ -1462,6 +1476,7 @@ public void pause(final boolean v) throws IOException { if (executable instanceof AccessControlled) { ((AccessControlled) executable).checkPermission(Item.CANCEL); } + done = Boolean.FALSE; Futures.addCallback(programPromise, new FutureCallback() { @Override public void onSuccess(CpsThreadGroup g) { if (v) { From d33c90c2355d8f426399c39e720eb6a057855fa9 Mon Sep 17 00:00:00 2001 From: Sam Van Oort Date: Fri, 16 Mar 2018 21:17:22 -0400 Subject: [PATCH 05/25] Testcases to verify final build state --- .../jenkinsci/plugins/workflow/cps/CpsFlowExecution.java | 5 +++-- .../jenkinsci/plugins/workflow/cps/FlowDurabilityTest.java | 7 +++++++ 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java b/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java index 9afc91545..dac6cf7d3 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java @@ -284,7 +284,8 @@ public class CpsFlowExecution extends FlowExecution implements BlockableResume { private transient List startNodesSerial; // used only between unmarshal and onLoad @GuardedBy("this") - private /* almost final*/ NavigableMap heads = new TreeMap(); + /* almost final*/ NavigableMap heads = new TreeMap(); // Non-private for unit tests + @SuppressFBWarnings({"IS_FIELD_NOT_GUARDED", "IS2_INCONSISTENT_SYNC"}) // irrelevant here private transient Map headsSerial; // used only between unmarshal and onLoad @@ -315,7 +316,7 @@ public class CpsFlowExecution extends FlowExecution implements BlockableResume { * {@link FlowExecution} gets loaded into memory for the build records that have been completed, * and for those we don't want to load the program state, so that check should be efficient. */ - private Boolean done; + Boolean done; // Only non-private for unit test use. /** * Groovy compiler with CPS+sandbox transformation correctly setup. diff --git a/src/test/java/org/jenkinsci/plugins/workflow/cps/FlowDurabilityTest.java b/src/test/java/org/jenkinsci/plugins/workflow/cps/FlowDurabilityTest.java index 6e6e59c05..89a212ed8 100644 --- a/src/test/java/org/jenkinsci/plugins/workflow/cps/FlowDurabilityTest.java +++ b/src/test/java/org/jenkinsci/plugins/workflow/cps/FlowDurabilityTest.java @@ -56,6 +56,7 @@ import java.nio.channels.FileChannel; import java.util.ArrayList; import java.util.List; +import java.util.Map; import java.util.Random; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; @@ -333,6 +334,12 @@ static void verifyCompletedCleanly(Jenkins j, WorkflowRun run) throws Exception Assert.assertFalse("Should always be able to retrieve script", StringUtils.isEmpty(cpsFlow.getScript())); Assert.assertNull("We should have no Groovy shell left or that's a memory leak", cpsFlow.getShell()); Assert.assertNull("We should have no Groovy shell left or that's a memory leak", cpsFlow.getTrustedShell()); + assert cpsFlow.done == Boolean.TRUE; + assert cpsFlow.isComplete(); + assert cpsFlow.heads.size() == 1; + Map.Entry finalHead = cpsFlow.heads.entrySet().iterator().next(); + assert finalHead.getValue().get() instanceof FlowEndNode; + Assert.assertEquals(cpsFlow.storage.getNode(finalHead.getValue().get().getId()), finalHead.getValue().get()); } verifyExecutionRemoved(run); From 1ad3eb4640a66046249755ab33cc2baea05f9f71 Mon Sep 17 00:00:00 2001 From: Sam Van Oort Date: Mon, 19 Mar 2018 18:31:09 -0400 Subject: [PATCH 06/25] Diagnostic logging when setNewHead called with null head --- .../plugins/workflow/cps/FlowHead.java | 31 ++++++++++--------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/workflow/cps/FlowHead.java b/src/main/java/org/jenkinsci/plugins/workflow/cps/FlowHead.java index 742e0281d..4ac8894b8 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/cps/FlowHead.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/cps/FlowHead.java @@ -115,28 +115,31 @@ void newStartNode(FlowStartNode n) throws IOException { } void setNewHead(FlowNode v) { + if (v == null) { + LOGGER.log(Level.WARNING, "FlowHead.setNewHead called on FlowHead id="+this.id+" with a null FlowNode, execution="+this.execution); + } try { if (this.head != null) { CpsFlowExecution.maybeAutoPersistNode(head); } execution.storage.storeNode(v, true); - v.addAction(new TimingAction()); - this.head = v; - CpsThreadGroup c = CpsThreadGroup.current(); - if (c !=null) { - // if the manipulation is from within the program executing thread, then - // defer the notification till we get to a safe point. - c.notifyNewHead(v); - } else { - // in recovering from error and such situation, we sometimes need to grow the graph - // without running the program. - // TODO can CpsThreadGroup.notifyNewHead be used instead to notify both kinds of listeners? - execution.notifyListeners(Collections.singletonList(v), true); - execution.notifyListeners(Collections.singletonList(v), false); - } } catch (IOException e) { LOGGER.log(Level.FINE, "Failed to record new head: " + v, e); } + v.addAction(new TimingAction()); + this.head = v; + CpsThreadGroup c = CpsThreadGroup.current(); + if (c !=null) { + // if the manipulation is from within the program executing thread, then + // defer the notification till we get to a safe point. + c.notifyNewHead(v); + } else { + // in recovering from error and such situation, we sometimes need to grow the graph + // without running the program. + // TODO can CpsThreadGroup.notifyNewHead be used instead to notify both kinds of listeners? + execution.notifyListeners(Collections.singletonList(v), true); + execution.notifyListeners(Collections.singletonList(v), false); + } } FlowNode get() { From cf829bb54b545afef29b1b1e019aab0a27062186 Mon Sep 17 00:00:00 2001 From: Sam Van Oort Date: Mon, 19 Mar 2018 19:10:51 -0400 Subject: [PATCH 07/25] Downgrade one warning log to info --- .../org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java b/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java index dac6cf7d3..e72a3194f 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java @@ -624,8 +624,8 @@ private void rebuildEmptyGraph() { } if (this.heads != null && this.heads.size() > 0) { - if (LOGGER.isLoggable(Level.WARNING)) { - LOGGER.log(Level.WARNING, "Resetting heads to rebuild the Pipeline structure, tossing existing heads: "+getHeadsAsString()); + if (LOGGER.isLoggable(Level.INFO)) { + LOGGER.log(Level.INFO, "Resetting heads to rebuild the Pipeline structure, tossing existing heads: "+getHeadsAsString()); } this.heads.clear(); } From a017be94477e5bdefbb234ed1024ec1dc704675a Mon Sep 17 00:00:00 2001 From: Sam Van Oort Date: Mon, 19 Mar 2018 19:17:34 -0400 Subject: [PATCH 08/25] Diagnostics fix --- .../org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java b/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java index e72a3194f..5c176bf15 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java @@ -1219,7 +1219,7 @@ synchronized void onProgramEnd(Outcome outcome) { } String tempIotaStr = Integer.toString(this.iota.get()); - FlowHead lastHead = heads.get(tempIotaStr); + FlowHead lastHead = heads.get(this.iota.get()); if (lastHead == null || lastHead.get() == null || !(lastHead.get().getId().equals(tempIotaStr))) { LOGGER.log(Level.WARNING, "Invalid final head for execution "+this.owner+" with head: "+lastHead); } From 5d08569babf0249ddd3b0723bcf5671df1e48ac7 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Mon, 26 Mar 2018 11:36:19 -0400 Subject: [PATCH 09/25] [JENKINS-50407] Trying to diagnose an NPE. --- .../workflow/cps/SandboxContinuable.java | 21 +++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/workflow/cps/SandboxContinuable.java b/src/main/java/org/jenkinsci/plugins/workflow/cps/SandboxContinuable.java index 84c3314e5..38d0f74c6 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/cps/SandboxContinuable.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/cps/SandboxContinuable.java @@ -2,6 +2,7 @@ import com.cloudbees.groovy.cps.Continuable; import com.cloudbees.groovy.cps.Outcome; +import groovy.lang.GroovyShell; import java.util.List; import org.jenkinsci.plugins.scriptsecurity.sandbox.RejectedAccessException; import org.jenkinsci.plugins.scriptsecurity.sandbox.groovy.GroovySandbox; @@ -29,19 +30,27 @@ class SandboxContinuable extends Continuable { public Outcome run0(final Outcome cn, final List categories) { try { CpsFlowExecution e = thread.group.getExecution(); - return GroovySandbox.runInSandbox(new Callable() { - @Override - public Outcome call() { + if (e == null) { + throw new IllegalStateException("no loaded execution"); + } + GroovyShell shell = e.getShell(); + if (shell == null) { + throw new IllegalStateException("no loaded shell in " + e); + } + GroovyShell trustedShell = e.getTrustedShell(); + if (trustedShell == null) { + throw new IllegalStateException("no loaded trustedShell in " + e); + } + return GroovySandbox.runInSandbox(() -> { Outcome outcome = SandboxContinuable.super.run0(cn, categories); RejectedAccessException x = findRejectedAccessException(outcome.getAbnormal()); if (x != null) { ScriptApproval.get().accessRejected(x, ApprovalContext.create()); } return outcome; - } }, new GroovyClassLoaderWhitelist(CpsWhitelist.get(), - e.getTrustedShell().getClassLoader(), - e.getShell().getClassLoader())); + trustedShell.getClassLoader(), + shell.getClassLoader())); } catch (RuntimeException e) { throw e; } catch (Exception e) { From 4c545d608a8a08fa5cc9755acb328b068b57370e Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Mon, 26 Mar 2018 11:38:06 -0400 Subject: [PATCH 10/25] Mention issue number in exception for easy tracking. --- .../jenkinsci/plugins/workflow/cps/SandboxContinuable.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/workflow/cps/SandboxContinuable.java b/src/main/java/org/jenkinsci/plugins/workflow/cps/SandboxContinuable.java index 38d0f74c6..f72ab558c 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/cps/SandboxContinuable.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/cps/SandboxContinuable.java @@ -31,15 +31,15 @@ public Outcome run0(final Outcome cn, final List categories) { try { CpsFlowExecution e = thread.group.getExecution(); if (e == null) { - throw new IllegalStateException("no loaded execution"); + throw new IllegalStateException("JENKINS-50407: no loaded execution"); } GroovyShell shell = e.getShell(); if (shell == null) { - throw new IllegalStateException("no loaded shell in " + e); + throw new IllegalStateException("JENKINS-50407: no loaded shell in " + e); } GroovyShell trustedShell = e.getTrustedShell(); if (trustedShell == null) { - throw new IllegalStateException("no loaded trustedShell in " + e); + throw new IllegalStateException("JENKINS-50407: no loaded trustedShell in " + e); } return GroovySandbox.runInSandbox(() -> { Outcome outcome = SandboxContinuable.super.run0(cn, categories); From b34e1f1a70567a50b8a42a4ed8231b4290256f89 Mon Sep 17 00:00:00 2001 From: Sam Van Oort Date: Thu, 29 Mar 2018 23:22:56 -0400 Subject: [PATCH 11/25] Consume latest WF-job snapshot and amend logging --- pom.xml | 2 +- .../jenkinsci/plugins/workflow/cps/CpsFlowExecution.java | 8 ++------ .../jenkinsci/plugins/workflow/cps/CpsThreadGroup.java | 1 - 3 files changed, 3 insertions(+), 8 deletions(-) diff --git a/pom.xml b/pom.xml index c33d6ef8b..d29e9213c 100644 --- a/pom.xml +++ b/pom.xml @@ -141,7 +141,7 @@ org.jenkins-ci.plugins.workflow workflow-job - 2.17 + 2.18-20180329.215807-4 test diff --git a/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java b/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java index 5c176bf15..e15ca5a81 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java @@ -1212,14 +1212,10 @@ synchronized void onProgramEnd(Outcome outcome) { FlowHead first = getFirstHead(); first.setNewHead(head); heads.clear(); - heads.put(first.getId(),first); - - if (heads.size() > 1) { - LOGGER.log(Level.WARNING, "Execution "+this.owner+" finished but had more than one final FlowNode where one expected, heads are: "+this.getHeadsAsString()); - } + heads.put(first.getId(), first); String tempIotaStr = Integer.toString(this.iota.get()); - FlowHead lastHead = heads.get(this.iota.get()); + FlowHead lastHead = heads.get(first.getId()); if (lastHead == null || lastHead.get() == null || !(lastHead.get().getId().equals(tempIotaStr))) { LOGGER.log(Level.WARNING, "Invalid final head for execution "+this.owner+" with head: "+lastHead); } diff --git a/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsThreadGroup.java b/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsThreadGroup.java index 70277a071..a130fbd7e 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsThreadGroup.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsThreadGroup.java @@ -357,7 +357,6 @@ private boolean run() { } else { stillRunnable |= t.isRunnable(); } - changed = true; } } From f4aad6103b13302ff3e120328d80b1e1f773a773 Mon Sep 17 00:00:00 2001 From: Sam Van Oort Date: Fri, 30 Mar 2018 10:01:13 -0400 Subject: [PATCH 12/25] More errorproofing in FlowHead --- .../java/org/jenkinsci/plugins/workflow/cps/FlowHead.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/workflow/cps/FlowHead.java b/src/main/java/org/jenkinsci/plugins/workflow/cps/FlowHead.java index 4ac8894b8..cccfd5b08 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/cps/FlowHead.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/cps/FlowHead.java @@ -45,6 +45,8 @@ import org.jenkinsci.plugins.workflow.graph.FlowNode; import org.jenkinsci.plugins.workflow.graph.FlowStartNode; +import javax.annotation.Nonnull; + /** * Growing tip of the node graph. * @@ -114,9 +116,9 @@ void newStartNode(FlowStartNode n) throws IOException { execution.storage.storeNode(head, false); } - void setNewHead(FlowNode v) { + void setNewHead(@Nonnull FlowNode v) { if (v == null) { - LOGGER.log(Level.WARNING, "FlowHead.setNewHead called on FlowHead id="+this.id+" with a null FlowNode, execution="+this.execution); + throw new IllegalArgumentException("FlowHead.setNewHead called on FlowHead id="+this.id+" with a null FlowNode, execution="+this.execution); } try { if (this.head != null) { From 16dcb359c52f6ea1e6a8b82e31c27b01d8f3a76c Mon Sep 17 00:00:00 2001 From: Sam Van Oort Date: Tue, 3 Apr 2018 10:51:53 -0400 Subject: [PATCH 13/25] Apply the noted review changes --- .../jenkinsci/plugins/workflow/cps/CpsFlowExecution.java | 6 +++--- .../java/org/jenkinsci/plugins/workflow/cps/FlowHead.java | 3 ++- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java b/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java index e15ca5a81..ec223b77e 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java @@ -316,7 +316,7 @@ public class CpsFlowExecution extends FlowExecution implements BlockableResume { * {@link FlowExecution} gets loaded into memory for the build records that have been completed, * and for those we don't want to load the program state, so that check should be efficient. */ - Boolean done; // Only non-private for unit test use. + Boolean done; // Only non-private for unit test use. /** * Groovy compiler with CPS+sandbox transformation correctly setup. @@ -601,7 +601,7 @@ int approximateNodeCount() { return iota.get(); } - /** For diagnostic purposes only. */ + /** For diagnostic purposes only, this logs current heads to assist with troubleshooting. */ private synchronized String getHeadsAsString() { NavigableMap myHeads = this.heads; if (myHeads == null) { @@ -857,7 +857,7 @@ private void loadProgramFailed(final Throwable problem, SettableFuture Date: Tue, 3 Apr 2018 19:32:42 -0400 Subject: [PATCH 14/25] Bump wf-job version to version with more hardening --- pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pom.xml b/pom.xml index 67008428f..55f1de23b 100644 --- a/pom.xml +++ b/pom.xml @@ -141,7 +141,7 @@ org.jenkins-ci.plugins.workflow workflow-job - 2.18-20180329.215807-4 + 2.18-20180403.233137-5 test From 5aa15b5a2c772d711433e8605ebcc522aca93130 Mon Sep 17 00:00:00 2001 From: Sam Van Oort Date: Tue, 3 Apr 2018 23:06:28 -0400 Subject: [PATCH 15/25] More robustness enhancements from fuzzing and extend/enhance fuzzing tests plus pull in latest WF-JOB fixes --- pom.xml | 2 +- .../workflow/cps/CpsFlowExecution.java | 30 +++-- .../workflow/cps/FlowDurabilityTest.java | 125 ++++++++++++------ 3 files changed, 105 insertions(+), 52 deletions(-) diff --git a/pom.xml b/pom.xml index 55f1de23b..050b8e816 100644 --- a/pom.xml +++ b/pom.xml @@ -141,7 +141,7 @@ org.jenkins-ci.plugins.workflow workflow-job - 2.18-20180403.233137-5 + 2.18-20180404.011344-6 test diff --git a/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java b/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java index ec223b77e..ec0a22f85 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java @@ -820,7 +820,6 @@ private void loadProgramFailed(final Throwable problem, SettableFuture Date: Wed, 4 Apr 2018 22:37:45 -0400 Subject: [PATCH 16/25] Fix durability mode setting in test --- pom.xml | 2 +- .../plugins/workflow/cps/FlowDurabilityTest.java | 16 ++++++++++++++-- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/pom.xml b/pom.xml index 050b8e816..a7f0080b7 100644 --- a/pom.xml +++ b/pom.xml @@ -141,7 +141,7 @@ org.jenkins-ci.plugins.workflow workflow-job - 2.18-20180404.011344-6 + 2.18-20180404.152425-7 test diff --git a/src/test/java/org/jenkinsci/plugins/workflow/cps/FlowDurabilityTest.java b/src/test/java/org/jenkinsci/plugins/workflow/cps/FlowDurabilityTest.java index f28eccae6..8f6e82c84 100644 --- a/src/test/java/org/jenkinsci/plugins/workflow/cps/FlowDurabilityTest.java +++ b/src/test/java/org/jenkinsci/plugins/workflow/cps/FlowDurabilityTest.java @@ -28,6 +28,7 @@ import org.jenkinsci.plugins.workflow.graphanalysis.NodeStepNamePredicate; import org.jenkinsci.plugins.workflow.job.WorkflowJob; import org.jenkinsci.plugins.workflow.job.WorkflowRun; +import org.jenkinsci.plugins.workflow.job.properties.DurabilityHintJobProperty; import org.jenkinsci.plugins.workflow.steps.StepDescriptor; import org.jenkinsci.plugins.workflow.support.storage.FlowNodeStorage; import org.jenkinsci.plugins.workflow.support.storage.BulkFlowNodeStorage; @@ -791,8 +792,7 @@ private WorkflowRun runFuzzerJob(JenkinsRule jrule, String jobName, FlowDurabili WorkflowJob job = jenkins.getItemByFullName(jobName, WorkflowJob.class); if (job == null) { // Job may already have been created job = jenkins.createProject(WorkflowJob.class, jobName); - TestDurabilityHintProvider provider = Jenkins.getInstance().getExtensionList(TestDurabilityHintProvider.class).get(0); - provider.registerHint(job.getFullName(), hint); + job.addProperty(new DurabilityHintJobProperty(hint)); job.setDefinition(new CpsFlowDefinition( "echo 'first'\n" + "def steps = [:]\n" + @@ -849,12 +849,18 @@ public void evaluate() throws Throwable { nodesOut.clear(); nodesOut.addAll(new DepthFirstScanner().allNodes(run.getExecution())); nodesOut.sort(FlowScanningUtils.ID_ORDER_COMPARATOR); + if (run.getExecution() != null) { + Assert.assertEquals(FlowDurabilityHint.MAX_SURVIVABILITY, run.getExecution().getDurabilityHint()); + } } }); story.addStep(new Statement() { @Override public void evaluate() throws Throwable { WorkflowRun run = story.j.jenkins.getItemByFullName(jobName, WorkflowJob.class).getLastBuild(); + if (run.getExecution() != null) { + Assert.assertEquals(FlowDurabilityHint.MAX_SURVIVABILITY, run.getExecution().getDurabilityHint()); + } if (run.isBuilding()) { try { story.j.waitUntilNoActivityUpTo(30_000); @@ -886,12 +892,18 @@ public void fuzzTimingNonDurable() throws Exception { public void evaluate() throws Throwable { WorkflowRun run = runFuzzerJob(story.j, jobName, FlowDurabilityHint.PERFORMANCE_OPTIMIZED); logStart[0] = JenkinsRule.getLog(run); + if (run.getExecution() != null) { + Assert.assertEquals(FlowDurabilityHint.PERFORMANCE_OPTIMIZED, run.getExecution().getDurabilityHint()); + } } }); story.addStep(new Statement() { @Override public void evaluate() throws Throwable { WorkflowRun run = story.j.jenkins.getItemByFullName(jobName, WorkflowJob.class).getLastBuild(); + if (run.getExecution() != null) { + Assert.assertEquals(FlowDurabilityHint.PERFORMANCE_OPTIMIZED, run.getExecution().getDurabilityHint()); + } if (run.isBuilding()) { try { story.j.waitUntilNoActivityUpTo(30_000); From e69a24ce80473a071a267a3b1eb7d3f7433a321d Mon Sep 17 00:00:00 2001 From: Sam Van Oort Date: Thu, 5 Apr 2018 16:35:27 -0400 Subject: [PATCH 17/25] Additional robustness improvements, plus a bit more test coverage --- pom.xml | 2 +- .../plugins/workflow/cps/CpsFlowExecution.java | 10 ++++------ .../plugins/workflow/cps/FlowDurabilityTest.java | 11 ++++++++--- 3 files changed, 13 insertions(+), 10 deletions(-) diff --git a/pom.xml b/pom.xml index a7f0080b7..feabcb397 100644 --- a/pom.xml +++ b/pom.xml @@ -69,7 +69,7 @@ 2.17 2.0.8 1.24 - 2.33 + 2.37-20180405.183013-2 diff --git a/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java b/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java index ec0a22f85..69bf0e821 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java @@ -316,7 +316,7 @@ public class CpsFlowExecution extends FlowExecution implements BlockableResume { * {@link FlowExecution} gets loaded into memory for the build records that have been completed, * and for those we don't want to load the program state, so that check should be efficient. */ - Boolean done; // Only non-private for unit test use. + boolean done; // Only non-private for unit test use. /** * Groovy compiler with CPS+sandbox transformation correctly setup. @@ -728,7 +728,7 @@ public void onLoad(FlowExecutionOwner owner) throws IOException { LOGGER.log(Level.WARNING, "Pipeline state not properly persisted, cannot resume "+owner.getUrl()); throw new IOException("Cannot resume build -- was not cleanly saved when Jenkins shut down."); } - } else if (done == Boolean.TRUE && !super.isComplete()) { + } else if (done && !super.isComplete()) { LOGGER.log(Level.WARNING, "Completed flow without FlowEndNode: "+this+" heads:"+getHeadsAsString()); } } catch (Exception e) { // Multicatch ensures that failure to load does not nuke the master @@ -1191,7 +1191,7 @@ public static void maybeAutoPersistNode(@Nonnull FlowNode node) { @Override @SuppressFBWarnings(value = "RC_REF_COMPARISON_BAD_PRACTICE_BOOLEAN", justification = "We want to explicitly check for boolean not-null and true") public boolean isComplete() { - return done == Boolean.TRUE || super.isComplete(); // Compare to Boolean.TRUE so null == false. + return done || super.isComplete(); // Compare to Boolean.TRUE so null == false. } /** @@ -1570,9 +1570,7 @@ public void marshal(Object source, HierarchicalStreamWriter w, MarshallingContex for (BlockStartNode st : e.startNodes) { writeChild(w, context, "start", st.getId(), String.class); } - if (e.done != null) { - writeChild(w, context, "done", e.done, Boolean.class); - } + writeChild(w, context, "done", e.done, Boolean.class); } writeChild(w, context, "resumeBlocked", e.resumeBlocked, Boolean.class); diff --git a/src/test/java/org/jenkinsci/plugins/workflow/cps/FlowDurabilityTest.java b/src/test/java/org/jenkinsci/plugins/workflow/cps/FlowDurabilityTest.java index 8f6e82c84..2f63fd0ac 100644 --- a/src/test/java/org/jenkinsci/plugins/workflow/cps/FlowDurabilityTest.java +++ b/src/test/java/org/jenkinsci/plugins/workflow/cps/FlowDurabilityTest.java @@ -165,6 +165,8 @@ static WorkflowRun createAndRunBasicJob(Jenkins jenkins, String jobName, FlowDur WorkflowRun run = job.scheduleBuild2(0).getStartCondition().get(); SemaphoreStep.waitForStart("halt/"+semaphoreIndex, run); Assert.assertEquals(durabilityHint, run.getExecution().getDurabilityHint()); + Assert.assertFalse(run.getExecution().isComplete()); + Assert.assertFalse(((CpsFlowExecution)(run.getExecution())).done); if (durabilityHint.isPersistWithEveryStep()) { assertBaseStorageType(run.getExecution(), SimpleXStreamFlowNodeStorage.class); } else { @@ -190,6 +192,8 @@ static WorkflowRun createAndRunSleeperJob(Jenkins jenkins, String jobName, FlowD job.setDefinition(def); WorkflowRun run = job.scheduleBuild2(0).getStartCondition().get(); Thread.sleep(4000L); // Hacky but we just need to ensure this can start up + Assert.assertFalse(run.getExecution().isComplete()); + Assert.assertFalse(((CpsFlowExecution)(run.getExecution())).done); Assert.assertEquals(durabilityHint, run.getExecution().getDurabilityHint()); Assert.assertEquals("sleep", run.getExecution().getCurrentHeads().get(0).getDisplayFunctionName()); return run; @@ -335,7 +339,7 @@ static void verifyCompletedCleanly(Jenkins j, WorkflowRun run) throws Exception Assert.assertFalse("Should always be able to retrieve script", StringUtils.isEmpty(cpsFlow.getScript())); Assert.assertNull("We should have no Groovy shell left or that's a memory leak", cpsFlow.getShell()); Assert.assertNull("We should have no Groovy shell left or that's a memory leak", cpsFlow.getTrustedShell()); - assert cpsFlow.done == Boolean.TRUE; + Assert.assertTrue(cpsFlow.done); assert cpsFlow.isComplete(); assert cpsFlow.heads.size() == 1; Map.Entry finalHead = cpsFlow.heads.entrySet().iterator().next(); @@ -833,12 +837,13 @@ private WorkflowRun runFuzzerJob(JenkinsRule jrule, String jobName, FlowDurabili /** Test interrupting build by randomly dying at unpredictable times. */ @Test - @Ignore //Too long to run as part of main suite + //Too long to run as part of main suite @TimedRepeatRule.RepeatForTime(repeatMillis = 170_000) public void fuzzTimingDurable() throws Exception { final String jobName = "NestedParallelDurableJob"; final String[] logStart = new String[1]; final List nodesOut = new ArrayList(); + final int[] buildNumber = new int [1]; // Create thread that eventually interrupts Jenkins with a hard shutdown at a random time interval story.addStepWithDirtyShutdown(new Statement() { @@ -864,10 +869,10 @@ public void evaluate() throws Throwable { if (run.isBuilding()) { try { story.j.waitUntilNoActivityUpTo(30_000); - Assert.assertEquals(Result.SUCCESS, run.getResult()); } catch (AssertionError ase) { throw new AssertionError("Build hung: "+run, ase); } + Assert.assertEquals(Result.SUCCESS, run.getResult()); } else { verifyCompletedCleanly(story.j.jenkins, run); } From 9d8f0b1d5ae24d1d4735f90f572316b0e61858f8 Mon Sep 17 00:00:00 2001 From: Sam Van Oort Date: Thu, 5 Apr 2018 17:00:05 -0400 Subject: [PATCH 18/25] Ensure that Program state always has its FlowNodes fully persisted before being saved --- .../plugins/workflow/cps/CpsFlowExecution.java | 14 +++++++------- .../plugins/workflow/cps/CpsStepContext.java | 7 +++++-- .../plugins/workflow/cps/CpsThreadGroup.java | 11 +++++++++++ 3 files changed, 23 insertions(+), 9 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java b/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java index 69bf0e821..dcd2db687 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java @@ -1820,6 +1820,13 @@ private void checkpoint() { boolean persistOk = true; FlowNodeStorage storage = getStorage(); if (storage != null) { + try { // Node storage must be flushed first so program can be restored + storage.flush(); + } catch (IOException ioe) { + persistOk=false; + LOGGER.log(Level.WARNING, "Error persisting FlowNode storage before shutdown", ioe); + } + // Try to ensure we've saved the appropriate things -- the program is the last stumbling block. try { final SettableFuture myOutcome = SettableFuture.create(); @@ -1851,13 +1858,6 @@ public void onFailure(Throwable t) { persistOk = false; LOGGER.log(Level.FINE, "Error saving program, that should be handled elsewhere.", ex); } - - try { - storage.flush(); - } catch (IOException ioe) { - persistOk=false; - LOGGER.log(Level.WARNING, "Error persisting FlowNode storage before shutdown", ioe); - } persistedClean = persistOk; saveOwner(); } diff --git a/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsStepContext.java b/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsStepContext.java index d6e83fdc4..d1195c358 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsStepContext.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsStepContext.java @@ -534,9 +534,12 @@ synchronized boolean switchToAsyncMode() { @Override public void onSuccess(CpsThreadGroup result) { try { // TODO keep track of whether the program was saved anyway after saveState was called but before now, and do not bother resaving it in that case - result.saveProgram(); + if (result.getExecution().getDurabilityHint().isPersistWithEveryStep()) { + result.getExecution().getStorage().flush(); + result.saveProgram(); + } f.set(null); - } catch (IOException x) { + } catch (Exception x) { f.setException(x); } } diff --git a/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsThreadGroup.java b/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsThreadGroup.java index a130fbd7e..fd0b8ae75 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsThreadGroup.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsThreadGroup.java @@ -68,6 +68,7 @@ import static org.jenkinsci.plugins.workflow.cps.CpsFlowExecution.*; import static org.jenkinsci.plugins.workflow.cps.persistence.PersistenceContext.*; import org.jenkinsci.plugins.workflow.pickles.PickleFactory; +import org.jenkinsci.plugins.workflow.support.storage.FlowNodeStorage; /** * List of {@link CpsThread}s that form a single {@link CpsFlowExecution}. @@ -425,6 +426,16 @@ public CpsThreadDump getThreadDump() { void saveProgramIfPossible(boolean enteringQuietState) { if (this.getExecution() != null && (this.getExecution().getDurabilityHint().isPersistWithEveryStep() || enteringQuietState)) { + + try { // Program may depend on flownodes being saved, so save nodes + FlowNodeStorage storage = this.execution.getStorage(); + if (storage != null) { + storage.flush(); + } + } catch (IOException ioe) { + LOGGER.log(Level.WARNING, "Error persisting FlowNode storage before saving program", ioe); + } + try { saveProgram(); } catch (IOException x) { From 95d7ee2c019eb2ceccf57d023d754d40b49243a2 Mon Sep 17 00:00:00 2001 From: Sam Van Oort Date: Thu, 5 Apr 2018 17:15:25 -0400 Subject: [PATCH 19/25] Add additional testcase Javadocs --- .../plugins/workflow/cps/FlowDurabilityTest.java | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/test/java/org/jenkinsci/plugins/workflow/cps/FlowDurabilityTest.java b/src/test/java/org/jenkinsci/plugins/workflow/cps/FlowDurabilityTest.java index 2f63fd0ac..b5f11972b 100644 --- a/src/test/java/org/jenkinsci/plugins/workflow/cps/FlowDurabilityTest.java +++ b/src/test/java/org/jenkinsci/plugins/workflow/cps/FlowDurabilityTest.java @@ -835,7 +835,9 @@ private WorkflowRun runFuzzerJob(JenkinsRule jrule, String jobName, FlowDurabili return run; } - /** Test interrupting build by randomly dying at unpredictable times. */ + /** Test interrupting build by randomly dying at unpredictable times. + * May fail rarely due to files being copied in a different order than they are modified as part of simulating a dirty restart. + * See {@link RestartableJenkinsRule#simulateAbruptShutdown()} for why that copying happens. */ @Test //Too long to run as part of main suite @TimedRepeatRule.RepeatForTime(repeatMillis = 170_000) @@ -883,7 +885,9 @@ public void evaluate() throws Throwable { } - /** Test interrupting build by randomly dying at unpredictable times. */ + /** Test interrupting build by randomly dying at unpredictable times. + * May fail rarely due to files being copied in a different order than they are modified as part of simulating a dirty restart. + * See {@link RestartableJenkinsRule#simulateAbruptShutdown()} for why that copying happens. */ @Test @Ignore //Too long to run as part of main suite @TimedRepeatRule.RepeatForTime(repeatMillis = 150_000) From a9c52d68a72e06bf443fba07a1b1f3b75312d5a0 Mon Sep 17 00:00:00 2001 From: Sam Van Oort Date: Thu, 5 Apr 2018 17:30:12 -0400 Subject: [PATCH 20/25] Missing ignore for fuzzer test --- .../org/jenkinsci/plugins/workflow/cps/FlowDurabilityTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/java/org/jenkinsci/plugins/workflow/cps/FlowDurabilityTest.java b/src/test/java/org/jenkinsci/plugins/workflow/cps/FlowDurabilityTest.java index b5f11972b..6900b5e6c 100644 --- a/src/test/java/org/jenkinsci/plugins/workflow/cps/FlowDurabilityTest.java +++ b/src/test/java/org/jenkinsci/plugins/workflow/cps/FlowDurabilityTest.java @@ -839,7 +839,7 @@ private WorkflowRun runFuzzerJob(JenkinsRule jrule, String jobName, FlowDurabili * May fail rarely due to files being copied in a different order than they are modified as part of simulating a dirty restart. * See {@link RestartableJenkinsRule#simulateAbruptShutdown()} for why that copying happens. */ @Test - //Too long to run as part of main suite + @Ignore //Too long to run as part of main suite @TimedRepeatRule.RepeatForTime(repeatMillis = 170_000) public void fuzzTimingDurable() throws Exception { final String jobName = "NestedParallelDurableJob"; From a4110d250de88d78a1be075b73a0624cc970d40c Mon Sep 17 00:00:00 2001 From: Sam Van Oort Date: Thu, 5 Apr 2018 18:14:27 -0400 Subject: [PATCH 21/25] Update test harness dep to latest snapshot --- pom.xml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pom.xml b/pom.xml index feabcb397..201f62e27 100644 --- a/pom.xml +++ b/pom.xml @@ -69,7 +69,8 @@ 2.17 2.0.8 1.24 - 2.37-20180405.183013-2 + + 2.37-20180405.213807-3 From 773855bd49e9e546116acd56ec1a2395cc044581 Mon Sep 17 00:00:00 2001 From: Sam Van Oort Date: Fri, 6 Apr 2018 09:43:47 -0400 Subject: [PATCH 22/25] Reduce one fuzz test timing so it cannot time out --- .../org/jenkinsci/plugins/workflow/cps/FlowDurabilityTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/java/org/jenkinsci/plugins/workflow/cps/FlowDurabilityTest.java b/src/test/java/org/jenkinsci/plugins/workflow/cps/FlowDurabilityTest.java index 6900b5e6c..dd5e95cd2 100644 --- a/src/test/java/org/jenkinsci/plugins/workflow/cps/FlowDurabilityTest.java +++ b/src/test/java/org/jenkinsci/plugins/workflow/cps/FlowDurabilityTest.java @@ -840,7 +840,7 @@ private WorkflowRun runFuzzerJob(JenkinsRule jrule, String jobName, FlowDurabili * See {@link RestartableJenkinsRule#simulateAbruptShutdown()} for why that copying happens. */ @Test @Ignore //Too long to run as part of main suite - @TimedRepeatRule.RepeatForTime(repeatMillis = 170_000) + @TimedRepeatRule.RepeatForTime(repeatMillis = 150_000) public void fuzzTimingDurable() throws Exception { final String jobName = "NestedParallelDurableJob"; final String[] logStart = new String[1]; From 91212d2aebf86ce354d7372a95c08f0a5872d1f0 Mon Sep 17 00:00:00 2001 From: Sam Van Oort Date: Fri, 6 Apr 2018 11:45:22 -0400 Subject: [PATCH 23/25] Fix one error logging that missed the exception --- .../org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java b/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java index dcd2db687..b34366d14 100644 --- a/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java +++ b/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java @@ -861,7 +861,7 @@ void croak(Throwable t) { try { saveOwner(); } catch (Exception ex) { - LOGGER.log(Level.WARNING, "Failed to persist WorkflowRun after noting a serious failure for run:", owner); + LOGGER.log(Level.WARNING, "Failed to persist WorkflowRun after noting a serious failure for run: " + owner, ex); } } From 5ecfd2c6d89ca24f6b680b3501c841d1828d4b30 Mon Sep 17 00:00:00 2001 From: Sam Van Oort Date: Fri, 6 Apr 2018 17:08:10 -0400 Subject: [PATCH 24/25] Update to test harness --- pom.xml | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/pom.xml b/pom.xml index 201f62e27..8ce24a580 100644 --- a/pom.xml +++ b/pom.xml @@ -69,8 +69,7 @@ 2.17 2.0.8 1.24 - - 2.37-20180405.213807-3 + 2.37 @@ -142,7 +141,7 @@ org.jenkins-ci.plugins.workflow workflow-job - 2.18-20180404.152425-7 + 2.18-20180406.172304-8 test From 1360644cdf44aedf3004a2e310baf8b92ef8c2dc Mon Sep 17 00:00:00 2001 From: Sam Van Oort Date: Sun, 8 Apr 2018 11:40:48 -0400 Subject: [PATCH 25/25] Consume final workflow-job released version --- pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pom.xml b/pom.xml index 8ce24a580..cc75b5b77 100644 --- a/pom.xml +++ b/pom.xml @@ -141,7 +141,7 @@ org.jenkins-ci.plugins.workflow workflow-job - 2.18-20180406.172304-8 + 2.18 test