[JENKINS-75310] Fix log recorder level updates

core/src/main/java/hudson/logging/LogRecorder.java

@@ -324,10 +324,14 @@
          * Makes sure that the logger passes through messages at the correct level to us.
          */
         public void enable() {
+            enableLogger();
+            new SetLevel(name, getLevel()).broadcast();
+        }
+
+        private void enableLogger() {
             Logger l = getLogger();
             if (!l.isLoggable(getLevel()))
                 l.setLevel(getLevel());
-            new SetLevel(name, getLevel()).broadcast();
         }
 
         public void disable() {
@@ -409,6 +413,63 @@
         return Jenkins.get().getLog();
     }
 
+    /**
+     * Updates controller logger levels after existing targets are reconfigured or removed. {@link Target#enable()}
+     * only makes a logger more verbose, so it cannot otherwise apply a change such as {@code FINEST} to {@code INFO}.
+     */
+    private void updateLogLevels(List<Target> previousTargets) {
+        List<LogRecorder> recorders = getParent().getRecorders();
+        Set<String> changedTargetNames = new LinkedHashSet<>();
+        for (Target previousTarget : previousTargets) {
+            Target target = getTarget(previousTarget.name, loggers);
+            if (target != null && target.getLevel().intValue() <= previousTarget.getLevel().intValue()) {
+                continue;
+            }
+            changedTargetNames.add(previousTarget.name);
+            if (sameLevel(previousTarget.getLogger().getLevel(), previousTarget.getLevel())) {
+                previousTarget.getLogger().setLevel(null);
+            }
+        }
+
+        for (LogRecorder recorder : recorders) {
+            for (Target target : recorder.loggers) {
+                if (changedTargetNames.contains(target.name)) {
+                    target.enableLogger();
+                }
+            }
+        }
+
+        List<Target> descendantTargets = new ArrayList<>();
+        for (LogRecorder recorder : recorders) {
+            for (Target target : recorder.loggers) {
+                for (String name : changedTargetNames) {
+                    if (isDescendant(name, target.name)) {
+                        descendantTargets.add(target);
+                        break;
+                    }
+                }
+            }
+        }
+        descendantTargets.sort(Comparator.comparingInt(target -> target.name.length()));
+        descendantTargets.forEach(Target::enableLogger);
+    }
+
+    private static Target getTarget(String loggerName, List<Target> targets) {
+        return targets.stream().filter(target -> target.name.equals(loggerName)).findFirst().orElse(null);
+    }
+
+    private static boolean isDescendant(String loggerName, String candidate) {
+        return loggerName.isEmpty()
+                ? !candidate.isEmpty()
+                : candidate.startsWith(loggerName)
+                        && candidate.length() > loggerName.length()
+                        && candidate.charAt(loggerName.length()) == '.';
+    }
+
+    private static boolean sameLevel(Level first, Level second) {
+        return first == null ? second == null : second != null && first.intValue() == second.intValue();
+    }
+
     /**
      * Accepts submission from the configuration page.
      */
@@ -431,10 +492,11 @@
             redirect = "../" + Util.rawEncode(newName) + '/';
         }
 
+        List<Target> previousTargets = List.copyOf(loggers);
         List<Target> newTargets = req.bindJSONToList(Target.class, src.get("loggers"));
         setLoggers(newTargets);
 
-        save();
+        save(previousTargets);
         if (oldFile != null) oldFile.delete();
         FormApply.success(redirect).generateResponse(req, rsp, null);
     }
@@ -461,10 +523,17 @@
      */
     @Override
     public synchronized void save() throws IOException {
+        save(null);
+    }
+
+    private synchronized void save(List<Target> previousTargets) throws IOException {
         if (BulkChange.contains(this))   return;
 
         getConfigFile().write(this);
         loggers.forEach(Target::enable);
+        if (previousTargets != null) {
+            updateLogLevels(previousTargets);
+        }
 
         SaveableListener.fireOnChange(this, getConfigFile());
     }

test/src/test/java/hudson/logging/LogRecorderManagerTest.java

@@ -33,7 +33,9 @@
 import static org.hamcrest.Matchers.hasSize;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotEquals;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNull;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
@@ -53,6 +55,7 @@
 import java.util.logging.SimpleFormatter;
 import jenkins.security.MasterToSlaveCallable;
 import org.htmlunit.FailingHttpStatusCodeException;
+import org.htmlunit.WebClientUtil;
 import org.htmlunit.html.HtmlForm;
 import org.htmlunit.html.HtmlPage;
 import org.junit.jupiter.api.BeforeEach;
@@ -149,6 +152,206 @@ void createLogRecorderWithNonAsciiName() throws Exception {
         j.createWebClient().goTo("log/" + Util.rawEncode(name) + "/configure");
     }
 
+    @Issue("JENKINS-75310")
+    @Test
+    void changingRecorderToLessVerboseLevelUpdatesLogger() throws Exception {
+        String loggerName = getClass().getName() + ".lessVerbose";
+        Logger logger = Logger.getLogger(loggerName);
+        LogRecorder recorder = new LogRecorder("recorder");
+        recorder.setLoggers(List.of(new LogRecorder.Target(loggerName, Level.FINEST)));
+        j.jenkins.getLog().getRecorders().add(recorder);
+
+        try {
+            recorder.save();
+            assertEquals(Level.FINEST, logger.getLevel());
+
+            configureLevel(recorder, Level.INFO);
+
+            assertNotEquals(Level.FINEST, logger.getLevel());
+            assertFalse(logger.isLoggable(Level.FINER));
+        } finally {
+            logger.setLevel(null);
+        }
+    }
+
+    @Issue("JENKINS-75310")
+    @Test
+    void changingParentTargetPreservesIntermediateManualLevel() throws Exception {
+        String parentLoggerName = getClass().getName() + ".manual";
+        String intermediateLoggerName = parentLoggerName + ".intermediate";
+        String childLoggerName = intermediateLoggerName + ".child";
+        Logger parentLogger = Logger.getLogger(parentLoggerName);
+        Logger intermediateLogger = Logger.getLogger(intermediateLoggerName);
+        Logger childLogger = Logger.getLogger(childLoggerName);
+        LogRecorder parent = new LogRecorder("manual-parent");
+        parent.setLoggers(List.of(new LogRecorder.Target(parentLoggerName, Level.FINE)));
+        LogRecorder child = new LogRecorder("manual-child");
+        child.setLoggers(List.of(new LogRecorder.Target(childLoggerName, Level.WARNING)));
+        j.jenkins.getLog().getRecorders().addAll(List.of(parent, child));
+
+        try {
+            parent.save();
+            child.save();
+            intermediateLogger.setLevel(Level.FINEST);
+            assertNull(childLogger.getLevel());
+
+            configureLevel(parent, Level.INFO);
+
+            assertFalse(parentLogger.isLoggable(Level.FINE));
+            assertEquals(Level.FINEST, intermediateLogger.getLevel());
+            assertNull(childLogger.getLevel());
+            assertTrue(childLogger.isLoggable(Level.FINER));
+        } finally {
+            parentLogger.setLevel(null);
+            intermediateLogger.setLevel(null);
+            childLogger.setLevel(null);
+        }
+    }
+
+    @Issue("JENKINS-75310")
+    @Test
+    void changingRecorderPreservesMoreVerboseSharedTarget() throws Exception {
+        String loggerName = getClass().getName() + ".shared";
+        Logger logger = Logger.getLogger(loggerName);
+        LogRecorder first = new LogRecorder("first");
+        first.setLoggers(List.of(new LogRecorder.Target(loggerName, Level.FINEST)));
+        LogRecorder second = new LogRecorder("second");
+        second.setLoggers(List.of(new LogRecorder.Target(loggerName, Level.FINE)));
+        j.jenkins.getLog().getRecorders().addAll(List.of(first, second));
+
+        try {
+            first.save();
+            second.save();
+            assertEquals(Level.FINEST, logger.getLevel());
+
+            configureLevel(first, Level.INFO);
+
+            assertEquals(Level.FINE, logger.getLevel());
+        } finally {
+            logger.setLevel(null);
+        }
+    }
+
+    @Issue("JENKINS-75310")
+    @Test
+    void changingChildTargetDoesNotPinInheritedRecorderLevel() throws Exception {
+        String parentLoggerName = getClass().getName() + ".sharedParent";
+        String childLoggerName = parentLoggerName + ".child";
+        Logger parentLogger = Logger.getLogger(parentLoggerName);
+        Logger childLogger = Logger.getLogger(childLoggerName);
+        LogRecorder parent = new LogRecorder("shared-parent");
+        parent.setLoggers(List.of(new LogRecorder.Target(parentLoggerName, Level.FINEST)));
+        LogRecorder child = new LogRecorder("shared-child");
+        child.setLoggers(List.of(new LogRecorder.Target(childLoggerName, Level.INFO)));
+        j.jenkins.getLog().getRecorders().addAll(List.of(parent, child));
+
+        try {
+            parent.save();
+            child.save();
+            assertNull(childLogger.getLevel());
+
+            configureLevel(child, Level.WARNING);
+
+            assertNull(childLogger.getLevel());
+            parent.delete();
+            assertNull(childLogger.getLevel());
+            assertFalse(childLogger.isLoggable(Level.FINER));
+        } finally {
+            parentLogger.setLevel(null);
+            childLogger.setLevel(null);
+        }
+    }
+
+    @Issue("JENKINS-75310")
+    @Test
+    void changingParentTargetKeepsChildTargetLoggable() throws Exception {
+        String parentLoggerName = getClass().getName() + ".parent";
+        String childLoggerName = parentLoggerName + ".child";
+        Logger parentLogger = Logger.getLogger(parentLoggerName);
+        Logger childLogger = Logger.getLogger(childLoggerName);
+        LogRecorder parent = new LogRecorder("parent");
+        parent.setLoggers(List.of(new LogRecorder.Target(parentLoggerName, Level.FINE)));
+        LogRecorder child = new LogRecorder("child");
+        child.setLoggers(List.of(new LogRecorder.Target(childLoggerName, Level.FINE)));
+        j.jenkins.getLog().getRecorders().addAll(List.of(parent, child));
+
+        try {
+            parent.save();
+            child.save();
+            assertNull(childLogger.getLevel());
+
+            configureLevel(parent, Level.INFO);
+
+            assertFalse(parentLogger.isLoggable(Level.FINE));
+            assertEquals(Level.FINE, childLogger.getLevel());
+        } finally {
+            parentLogger.setLevel(null);
+            childLogger.setLevel(null);
+        }
+    }
+
+    @Issue("JENKINS-75310")
+    @Test
+    void removingRecorderTargetClearsLoggerLevel() throws Exception {
+        String loggerName = getClass().getName() + ".removed";
+        Logger logger = Logger.getLogger(loggerName);
+        LogRecorder recorder = new LogRecorder("removed");
+        recorder.setLoggers(List.of(new LogRecorder.Target(loggerName, Level.FINEST)));
+        j.jenkins.getLog().getRecorders().add(recorder);
+
+        try {
+            recorder.save();
+            assertEquals(Level.FINEST, logger.getLevel());
+
+            removeTarget(recorder);
+
+            assertTrue(recorder.getLoggers().isEmpty());
+            assertNull(logger.getLevel());
+        } finally {
+            logger.setLevel(null);
+        }
+    }
+
+    @Issue("JENKINS-75310")
+    @Test
+    void removingRecorderTargetPreservesSharedTarget() throws Exception {
+        String loggerName = getClass().getName() + ".removedShared";
+        Logger logger = Logger.getLogger(loggerName);
+        LogRecorder first = new LogRecorder("removed-first");
+        first.setLoggers(List.of(new LogRecorder.Target(loggerName, Level.FINEST)));
+        LogRecorder second = new LogRecorder("removed-second");
+        second.setLoggers(List.of(new LogRecorder.Target(loggerName, Level.FINE)));
+        j.jenkins.getLog().getRecorders().addAll(List.of(first, second));
+
+        try {
+            first.save();
+            second.save();
+            assertEquals(Level.FINEST, logger.getLevel());
+
+            removeTarget(first);
+
+            assertEquals(Level.FINE, logger.getLevel());
+        } finally {
+            logger.setLevel(null);
+        }
+    }
+
+    private void configureLevel(LogRecorder recorder, Level level) throws Exception {
+        HtmlPage page = j.createWebClient().goTo("log/" + Util.rawEncode(recorder.getName()) + "/configure");
+        HtmlForm form = page.getFormByName("config");
+        form.getSelectByName("level").getOptionByValue(level.getName()).setSelected(true);
+        j.submit(form);
+    }
+
+    private void removeTarget(LogRecorder recorder) throws Exception {
+        JenkinsRule.WebClient webClient = j.createWebClient();
+        HtmlPage page = webClient.goTo("log/" + Util.rawEncode(recorder.getName()) + "/configure");
+        HtmlForm form = page.getFormByName("config");
+        j.getButtonByCaption(form, "Delete").click();
+        WebClientUtil.waitForJSExec(webClient);
+        j.submit(form);
+    }
+
     @Issue({"JENKINS-18274", "JENKINS-63458"})
     @Test
     void loggingOnSlaves() throws Exception {