ErrorAction.findOrigin failures due to ProxyException

pom.xml

@@ -73,7 +73,7 @@
             <dependency>
                 <groupId>io.jenkins.tools.bom</groupId>
                 <artifactId>bom-2.426.x</artifactId>
-                <version>2555.v3190a_8a_c60c6</version>
+                <version>2745.vc7b_fe4c876fa_</version>
                 <scope>import</scope>
                 <type>pom</type>
             </dependency>

src/main/java/org/jenkinsci/plugins/workflow/actions/ErrorAction.java

@@ -37,12 +37,14 @@
 import java.util.Objects;
 import java.util.Set;
 import java.util.UUID;
+import java.util.logging.Level;
+import java.util.logging.Logger;
 import jenkins.model.Jenkins;
 import org.apache.commons.io.output.NullOutputStream;
 import org.jenkinsci.plugins.workflow.flow.FlowExecution;
 import org.jenkinsci.plugins.workflow.graph.AtomNode;
 import org.jenkinsci.plugins.workflow.graph.BlockEndNode;
-import org.jenkinsci.plugins.workflow.graphanalysis.ForkScanner;
+import org.jenkinsci.plugins.workflow.graphanalysis.DepthFirstScanner;
 import org.kohsuke.accmod.Restricted;
 import org.kohsuke.accmod.restrictions.Beta;
 
@@ -53,23 +55,32 @@
  */
 public class ErrorAction implements PersistentAction {
 
+    private static final Logger LOGGER = Logger.getLogger(ErrorAction.class.getName());
+
     private final @NonNull Throwable error;
 
     public ErrorAction(@NonNull Throwable error) {
+        Throwable errorForAction = error;
         if (isUnserializableException(error, new HashSet<>())) {
-            error = new ProxyException(error);
+            LOGGER.log(Level.FINE, "sanitizing unserializable error", error);
+            errorForAction = new ProxyException(error);
         } else if (error != null) {
             try {
                 Jenkins.XSTREAM2.toXMLUTF8(error, new NullOutputStream());
             } catch (Exception x) {
+                LOGGER.log(Level.FINE, "unable to serialize to XML", x);
                 // Typically SecurityException from ClassFilter.
-                error = new ProxyException(error);
+                errorForAction = new ProxyException(error);
             }
         }
-        this.error = error;
+        this.error = errorForAction;
         String id = findId(error, new HashSet<>());
-        if (id == null && error != null) {
-            error.addSuppressed(new ErrorId());
+        if (id == null) {
+            errorForAction.addSuppressed(new ErrorId());
+            if (error != errorForAction) {
+                // Make sure the original exception has the error ID, not just the copy here.
+                error.addSuppressed(new ErrorId());
+            }
         }
     }
 
@@ -140,7 +151,7 @@
      */
     public static @CheckForNull FlowNode findOrigin(@NonNull Throwable error, @NonNull FlowExecution execution) {
         FlowNode candidate = null;
-        for (FlowNode n : new ForkScanner().allNodes(execution)) {
+        for (FlowNode n : new DepthFirstScanner().allNodes(execution)) {
             ErrorAction errorAction = n.getPersistentAction(ErrorAction.class);
             if (errorAction != null && equals(error, errorAction.getError())) {
                 candidate = n; // continue search for earlier one
@@ -170,28 +181,48 @@
     @Restricted(Beta.class)
     public static boolean equals(Throwable t1, Throwable t2) {
         if (t1 == t2) {
+            LOGGER.fine(() -> "Same object: " + t1);
             return true;
-        } else if (t1.getClass() != t2.getClass()) {
+        }
+        String id1 = findId(t1, new HashSet<>());
+        if (id1 != null) {
+            String id2 = findId(t2, new HashSet<>());
+            if (id1.equals(id2)) {
+                LOGGER.fine(() -> "ErrorId matches: " + id1);
+                return true;
+            }
+            LOGGER.fine(() -> "ErrorId mismatch: " + t1 + " " + id1 + " vs. " + t2 + " " + id2);
+        } else {
+            LOGGER.fine(() -> "No ErrorId on " + t1);
+        }
+        if (t1.getClass() != t2.getClass()) {
+            LOGGER.fine(() -> "Different types: " + t1.getClass() + " vs. " + t2.getClass());
             return false;
         } else if (!Objects.equals(t1.getMessage(), t2.getMessage())) {
+            LOGGER.fine(() -> "Different messages: " + t1.getMessage() + " vs. " + t2.getMessage());
             return false;
         } else {
-            String id1 = findId(t1, new HashSet<>());
-            if (id1 != null) {
-                return id1.equals(findId(t2, new HashSet<>()));
-            }
             // No ErrorId, use a best-effort approach that doesn't work across restarts for exceptions thrown
             // synchronously from the CPS VM thread.
             // Check that stack traces match, but specifically avoid checking suppressed exceptions, which are often
             // modified after ErrorAction is written to disk when steps like parallel are involved.
-            while (t1 != null && t2 != null) {
-                if (!Arrays.equals(t1.getStackTrace(), t2.getStackTrace())) {
+            var _t1 = t1;
+            var _t2 = t2;
+            while (_t1 != null && _t2 != null) {
+                if (!Arrays.equals(_t1.getStackTrace(), _t2.getStackTrace())) {
+                    LOGGER.fine(() -> "Different stack traces between " + t1 + " vs. " + t2); // not showing details
                     return false;
                 }
-                t1 = t1.getCause();
-                t2 = t2.getCause();
+                _t1 = _t1.getCause();
+                _t2 = _t2.getCause();
+            }
+            if ((_t1 == null) == (_t2 == null)) {
+                LOGGER.fine(() -> "Same stack traces in " + t1 + " vs. " + t2);
+                return true;
+            } else {
+                LOGGER.fine(() -> "Different cause depths between " + t1 + " vs. " + t2);
+                return false;
             }
-            return (t1 == null) == (t2 == null);
         }
     }
 

src/test/java/org/jenkinsci/plugins/workflow/actions/ErrorActionTest.java

@@ -47,21 +47,46 @@
 import org.jvnet.hudson.test.JenkinsSessionRule;
 
 import groovy.lang.MissingMethodException;
+import hudson.FilePath;
 import hudson.Functions;
 import hudson.model.Result;
+import hudson.model.TaskListener;
 import hudson.remoting.ProxyException;
+import hudson.remoting.VirtualChannel;
+import java.io.File;
+import java.io.IOException;
+import java.util.Set;
+import java.util.logging.Level;
+import jenkins.MasterToSlaveFileCallable;
 import org.codehaus.groovy.runtime.NullObject;
+import org.jenkinsci.plugins.workflow.steps.BodyExecutionCallback;
+import org.jenkinsci.plugins.workflow.steps.Step;
+import org.jenkinsci.plugins.workflow.steps.StepContext;
+import org.jenkinsci.plugins.workflow.steps.StepDescriptor;
+import org.jenkinsci.plugins.workflow.steps.StepExecution;
+import org.jenkinsci.plugins.workflow.steps.StepExecutions;
+import org.jvnet.hudson.test.InboundAgentRule;
+import org.jvnet.hudson.test.LoggerRule;
+import org.jvnet.hudson.test.TestExtension;
+import org.kohsuke.stapler.DataBoundConstructor;
+import org.jenkinsci.plugins.workflow.graph.StepNode;
+import static org.hamcrest.Matchers.equalTo;
 
 /**
  * Tests for {@link ErrorAction}
  */
 public class ErrorActionTest {
+
     @ClassRule
     public static BuildWatcher buildWatcher = new BuildWatcher();
 
     @Rule
     public JenkinsSessionRule rr = new JenkinsSessionRule();
 
+    @Rule public InboundAgentRule agents = new InboundAgentRule();
+
+    @Rule public LoggerRule logging = new LoggerRule().record(ErrorAction.class, Level.FINE);
+
     private List<ErrorAction> extractErrorActions(FlowExecution exec) {
         List<ErrorAction> ret = new ArrayList<>();
 
@@ -228,6 +253,87 @@ public static class X extends Exception {
         });
     }
 
+    @Test public void findOriginFromBodyExecutionCallback() throws Throwable {
+        rr.then(r -> {
+            agents.createAgent(r, "remote");
+            var p = r.createProject(WorkflowJob.class);
+            p.setDefinition(new CpsFlowDefinition("callsFindOrigin {node('remote') {fails()}}", true));
+            var b = p.scheduleBuild2(0).waitForStart();
+            r.waitForMessage("Acting slowly in ", b);
+            agents.stop("remote");
+            r.assertBuildStatus(Result.FAILURE, r.waitForCompletion(b));
+            r.assertLogContains("Found in: fails", b);
+        });
+    }
+    public static final class WrapperStep extends Step {
+        @DataBoundConstructor public WrapperStep() {}
+        @Override public StepExecution start(StepContext context) throws Exception {
+            return new ExecutionImpl(context);
+        }
+        private static final class ExecutionImpl extends StepExecution {
+            ExecutionImpl(StepContext context) {
+                super(context);
+            }
+            @Override public boolean start() throws Exception {
+                getContext().newBodyInvoker().withCallback(new Callback()).start();
+                return false;
+            }
+        }
+        private static class Callback extends BodyExecutionCallback {
+            @Override public void onSuccess(StepContext context, Object result) {
+                context.onSuccess(result);
+            }
+            @Override
+            public void onFailure(StepContext context, Throwable t) {
+                try {
+                    var l = context.get(TaskListener.class);
+                    Functions.printStackTrace(t, l.error("Original failure:"));
+                    l.getLogger().println("Found in: " + ErrorAction.findOrigin(t, context.get(FlowExecution.class)).getDisplayFunctionName());
+                } catch (Exception x) {
+                    assert false : x;
+                }
+                context.onFailure(t);
+            }
+        }
+        @TestExtension("findOriginFromBodyExecutionCallback") public static final class DescriptorImpl extends StepDescriptor {
+            @Override public String getFunctionName() {
+                return "callsFindOrigin";
+            }
+            @Override public Set<? extends Class<?>> getRequiredContext() {
+                return Set.of();
+            }
+            @Override public boolean takesImplicitBlockArgument() {
+                return true;
+            }
+        }
+    }
+    public static final class FailingStep extends Step {
+        @DataBoundConstructor public FailingStep() {}
+        @Override public StepExecution start(StepContext context) throws Exception {
+            return StepExecutions.synchronousNonBlockingVoid(context, c -> c.get(FilePath.class).act(new Sleep(c.get(TaskListener.class))));
+        }
+        private static final class Sleep extends MasterToSlaveFileCallable<Void> {
+            private final TaskListener l;
+            Sleep(TaskListener l) {
+                this.l = l;
+            }
+            @Override public Void invoke(File f, VirtualChannel channel) throws IOException, InterruptedException {
+                l.getLogger().println("Acting slowly in " + f);
+                l.getLogger().flush();
+                Thread.sleep(Long.MAX_VALUE);
+                return null;
+            }
+        }
+        @TestExtension("findOriginFromBodyExecutionCallback") public static final class DescriptorImpl extends StepDescriptor {
+            @Override public String getFunctionName() {
+                return "fails";
+            }
+            @Override public Set<? extends Class<?>> getRequiredContext() {
+                return Set.of(FilePath.class);
+            }
+        }
+    }
+
     @Test public void cyclicErrorsAreSupported() throws Throwable {
         Exception cyclic1 = new Exception();
         Exception cyclic2 = new Exception(cyclic1);
@@ -243,4 +349,15 @@ public static class X extends Exception {
         assertNotNull(new ErrorAction(unserializable));
         assertNotNull(new ErrorAction(cyclic));
     }
+
+    @Test public void findOriginOfRethrownUnserializableException() throws Throwable {
+        rr.then(r -> {
+            WorkflowJob p = r.jenkins.createProject(WorkflowJob.class, "p");
+            p.setDefinition(new CpsFlowDefinition(
+                "stage('test') { withEnv([]) {  throw new " + X.class.getCanonicalName() + "() } }", false /* for "new org.jenkinsci.plugins.workflow.actions.ErrorActionTest$X" */));
+            WorkflowRun b = r.buildAndAssertStatus(Result.FAILURE, p);
+            FlowNode originNode = ErrorAction.findOrigin(b.getExecution().getCauseOfFailure(), b.getExecution());
+            assertThat(((StepNode) originNode).getDescriptor().getFunctionName(), equalTo("withEnv"));
+        });
+    }
 }