[JENKINS-57111] Do not put agent offline for exceptions in onOnline

Agent implementers use onOnline to perform actions after an agent
put online.  The API specifies that execeptions thrown by this
method will not put an agent offline.  However, the default
implementation does exactly that.

This change clarifies the API to say all exceptions will be logged
and hardens setChannel against exceptions during onOnline.

NOTE: Errors will still cause an agent to go offline as they are
generally fatal conditions.

Also added tests to verify the behavior.

core/src/main/java/hudson/slaves/ComputerListener.java

@@ -139,6 +139,11 @@ public abstract class ComputerListener implements ExtensionPoint {
      * This enables you to do some work on all the agents
      * as they get connected.
      *
+     * Any thrown {@link Exception}s will be recorded to the listener.
+     * No {@link Exception} will put the computer offline, however
+     * any {@link Error} will put the computer offline
+     * since they indicate unrecoverable conditions.
+     *
      * <p>
      * Starting Hudson 1.312, this method is also invoked for the master, not just for agents.
      *

core/src/main/java/hudson/slaves/SlaveComputer.java

@@ -694,7 +694,19 @@ public class SlaveComputer extends Computer {
         old = ACL.impersonate(ACL.SYSTEM);
         try {
             for (ComputerListener cl : ComputerListener.all()) {
-                cl.onOnline(this,taskListener);
+                try {
+                    cl.onOnline(this,taskListener);
+                } catch (Exception e) {
+                    // Per Javadoc log exceptions but still go online.
+                    // NOTE: this does not include Errors, which indicate a fatal problem
+                    taskListener.getLogger().format(
+                        "onOnline: %s reported an exception: %s%n",
+                        cl.getClass(),
+                        e.toString());
+                } catch (Throwable e) {
+                    closeChannel();
+                    throw e;
+                }
             }
         } finally {
             SecurityContextHolder.setContext(old);

core/src/main/java/jenkins/model/Jenkins.java

@@ -979,16 +979,11 @@ public class Jenkins extends AbstractCIBase implements DirectlyModifiableTopLeve
                     for (ComputerListener cl : ComputerListener.all()) {
                         try {
                             cl.onOnline(c, new LogTaskListener(LOGGER, INFO));
-                        } catch (Throwable t) {
-                            if (t instanceof Error) {
-                                // We propagate Runtime errors, because they are fatal.
-                                throw t;
-                            }
-
-                            // Other exceptions should be logged instead of failing the Jenkins startup (See listener's Javadoc)
-                            // We also throw it for InterruptedException since it's what is expected according to the javadoc
-                            LOGGER.log(SEVERE, String.format("Invocation of the computer listener %s failed for the Jenkins master node",
-                                    cl.getClass()), t);
+                        } catch (Exception e) {
+                            // Per Javadoc log exceptions but still go online.
+                            // NOTE: this does not include Errors, which indicate a fatal problem
+                            LOGGER.log(WARNING, String.format("Exception in onOnline() for the computer listener %s on the Jenkins master node",
+                                    cl.getClass()), e);
                         }
                     }
                 }

test/src/test/java/hudson/slaves/SlaveComputerTest.java

@@ -27,17 +27,21 @@ import com.gargoylesoftware.htmlunit.WebResponse;
 import hudson.model.*;
 import hudson.security.ACL;
 import hudson.security.ACLContext;
+import java.io.IOError;
+import java.io.IOException;
+import java.util.logging.Level;
 import jenkins.model.Jenkins;
 import net.sf.json.JSONNull;
 import net.sf.json.JSONObject;
 import org.junit.Assert;
 import org.junit.Rule;
 import org.junit.Test;
+import org.jvnet.hudson.test.Issue;
 import org.jvnet.hudson.test.JenkinsRule;
 import org.jvnet.hudson.test.MockAuthorizationStrategy;
+import org.jvnet.hudson.test.TestExtension;
 import org.xml.sax.SAXException;
 
-import java.io.IOException;
 
 /**
  * @author suren
@@ -77,6 +81,91 @@ public class SlaveComputerTest {
         }
     }
 
+    @Test
+    @Issue("JENKINS-57111")
+    public void startupShouldNotFailOnExceptionOnlineListener() throws Exception {
+        DumbSlave nodeA = j.createOnlineSlave();
+        Assert.assertTrue(nodeA.getComputer() instanceof SlaveComputer);
+
+        int retries = 10;
+        while (IOExceptionOnOnlineListener.onOnlineCount == 0 && retries > 0) {
+            retries--;
+            Thread.sleep(500);
+        }
+        Assert.assertTrue(retries > 0);
+        Thread.sleep(500);
+
+        Assert.assertFalse(nodeA.getComputer().isOffline());
+        Assert.assertTrue(nodeA.getComputer().isOnline());
+
+        // Both listeners should fire and not cause the other not to fire.
+        Assert.assertEquals(1, IOExceptionOnOnlineListener.onOnlineCount);
+        Assert.assertEquals(1, RuntimeExceptionOnOnlineListener.onOnlineCount);
+    }
+
+    @TestExtension(value = "startupShouldNotFailOnExceptionOnlineListener")
+    public static final class IOExceptionOnOnlineListener extends ComputerListener {
+
+        static int onOnlineCount = 0;
+
+        @Override
+        public void onOnline(Computer c, TaskListener listener) throws IOException, InterruptedException {
+            if (c instanceof SlaveComputer) {
+                onOnlineCount++;
+                throw new IOException("Something happened (the listener always throws this exception)");
+            }
+        }
+    }
+
+    @TestExtension(value = "startupShouldNotFailOnExceptionOnlineListener")
+    public static final class RuntimeExceptionOnOnlineListener extends ComputerListener {
+
+        static int onOnlineCount = 0;
+
+        @Override
+        public void onOnline(Computer c, TaskListener listener) throws IOException, InterruptedException {
+            if (c instanceof SlaveComputer) {
+                onOnlineCount++;
+                throw new RuntimeException("Something happened (the listener always throws this exception)");
+            }
+        }
+    }
+
+    @Test
+    @Issue("JENKINS-57111")
+
+    public void startupShouldFailOnErrorOnlineListener() throws Exception {
+        DumbSlave nodeA = j.createSlave();
+        Assert.assertTrue(nodeA.getComputer() instanceof SlaveComputer);
+        int retries = 10;
+        while (ErrorOnOnlineListener.onOnlineCount == 0 && retries > 0) {
+            retries--;
+            Thread.sleep(500);
+        }
+        Assert.assertTrue(retries > 0);
+        Thread.sleep(500);
+
+        Assert.assertEquals(1, ErrorOnOnlineListener.onOnlineCount);
+
+        Assert.assertTrue(nodeA.getComputer().isOffline());
+        Assert.assertFalse(nodeA.getComputer().isOnline());
+    }
+
+    @TestExtension(value = "startupShouldFailOnErrorOnlineListener")
+    public static final class ErrorOnOnlineListener extends ComputerListener {
+
+        static int onOnlineCount = 0;
+
+        @Override
+        public void onOnline(Computer c, TaskListener listener) throws IOException, InterruptedException {
+            if (c instanceof SlaveComputer) {
+                onOnlineCount++;
+                throw new IOError(new Exception("Something happened (the listener always throws this exception)"));
+            }
+        }
+    }
+
+
     /**
      * Get remote path through json api
      * @param node slave node

test/src/test/java/jenkins/model/JenkinsTest.java

@@ -81,6 +81,7 @@ import org.kohsuke.stapler.HttpResponse;
 import org.mockito.ArgumentCaptor;
 import org.mockito.Mockito;
 
+import java.io.IOError;
 import java.io.IOException;
 import java.net.HttpURLConnection;
 import java.net.Socket;
@@ -472,20 +473,45 @@ public class JenkinsTest {
 
     @Test
     @Issue("JENKINS-38487")
-    public void startupShouldNotFailOnFailingOnlineListener() {
-        // We do nothing, FailingOnOnlineListener & JenkinsRule should cause the 
+    public void startupShouldNotFailOnIOExceptionOnlineListener() {
+        // We do nothing, IOExceptionOnOnlineListener & JenkinsRule should cause the
         // boot failure if the issue is not fixed.
+
+        assertEquals(1, IOExceptionOnOnlineListener.onOnlineCount);
     }
 
-    @TestExtension(value = "startupShouldNotFailOnFailingOnlineListener")
-    public static final class FailingOnOnlineListener extends ComputerListener {
-        
+    @TestExtension(value = "startupShouldNotFailOnIOExceptionOnlineListener")
+    public static final class IOExceptionOnOnlineListener extends ComputerListener {
+
+        static int onOnlineCount = 0;
+
         @Override
         public void onOnline(Computer c, TaskListener listener) throws IOException, InterruptedException {
+            onOnlineCount++;
             throw new IOException("Something happened (the listener always throws this exception)");
         }
     }
-    
+
+    @Test
+    @Issue("JENKINS-57111")
+    public void startupShouldNotFailOnRuntimeExceptionOnlineListener() {
+        // We do nothing, RuntimeExceptionOnOnlineListener & JenkinsRule should cause the
+        // boot failure if the issue is not fixed.
+        assertEquals(1, RuntimeExceptionOnOnlineListener.onOnlineCount);
+    }
+
+    @TestExtension(value = "startupShouldNotFailOnRuntimeExceptionOnlineListener")
+    public static final class RuntimeExceptionOnOnlineListener extends ComputerListener {
+
+        static int onOnlineCount = 0;
+
+        @Override
+        public void onOnline(Computer c, TaskListener listener) throws IOException, InterruptedException {
+            onOnlineCount++;
+            throw new RuntimeException("Something happened (the listener always throws this exception)");
+        }
+    }
+
     @Test
     @Issue("JENKINS-39465")
     public void agentProtocols_singleEnable_roundtrip() throws Exception {