[JENKINS-73835] Do not allow builds to be deleted while they are stil…

…l running and ensure build discarders run after builds are fully complete (jenkinsci#9810)

* [JENKINS-73835] Do not allow builds to be deleted while they are still running

* [JENKINS-73835] Avoid redundant calls to Job.logRotate when builds complete and always call Job.logRotate after build finalization

* [JENKINS-73835] Add issue reference to RunTest.buildsMayNotBeDeletedWhileRunning

* [JENKINS-73835] Adjust DeleteBuildsCommandTest.deleteBuildsShouldSuccessEvenTheBuildIsRunning to match new behavior

* [JENKINS-73835] Run/delete.jelly should check Run.isLogUpdated, not Run.isBuilding

core/src/main/java/hudson/model/Run.java

@@ -1551,6 +1551,9 @@ public synchronized void deleteArtifacts() throws IOException {
      *      if we fail to delete.
      */
     public void delete() throws IOException {
+        if (isLogUpdated()) {
+            throw new IOException("Unable to delete " + this + " because it is still running");
+        }
         synchronized (this) {
             // Avoid concurrent delete. See https://issues.jenkins.io/browse/JENKINS-61687
             if (isPendingDelete) {
@@ -1885,12 +1888,6 @@ protected final void execute(@NonNull RunExecution job) {
                     LOGGER.log(Level.SEVERE, "Failed to save build record", e);
                 }
             }
-
-            try {
-                getParent().logRotate();
-            } catch (Exception e) {
-                LOGGER.log(Level.SEVERE, "Failed to rotate log", e);
-            }
         } finally {
             onEndBuilding();
             if (logger != null) {

core/src/main/java/hudson/tasks/LogRotator.java

@@ -250,7 +250,7 @@ private boolean shouldKeepRun(Run r, Run lsb, Run lstb) {
             LOGGER.log(FINER, "{0} is not to be removed or purged of artifacts because it’s the last stable build", r);
             return true;
         }
-        if (r.isBuilding()) {
+        if (r.isLogUpdated()) {
             LOGGER.log(FINER, "{0} is not to be removed or purged of artifacts because it’s still building", r);
             return true;
         }

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

@@ -31,6 +31,7 @@
 import java.io.IOException;
 import java.util.logging.Level;
 import java.util.logging.Logger;
+import java.util.stream.Stream;
 import org.kohsuke.accmod.Restricted;
 import org.kohsuke.accmod.restrictions.NoExternalUse;
 
@@ -56,8 +57,18 @@ protected void execute(TaskListener listener) throws IOException, InterruptedExc
         }
     }
 
+    /**
+     * Runs all globally configured build discarders against a job.
+     */
     public static void processJob(TaskListener listener, Job job) {
-        GlobalBuildDiscarderConfiguration.get().getConfiguredBuildDiscarders().forEach(strategy -> {
+        processJob(listener, job, GlobalBuildDiscarderConfiguration.get().getConfiguredBuildDiscarders().stream());
+    }
+
+    /**
+     * Runs the specified build discarders against a job.
+     */
+    public static void processJob(TaskListener listener, Job job, Stream<GlobalBuildDiscarderStrategy> strategies) {
+        strategies.forEach(strategy -> {
             String displayName = strategy.getDescriptor().getDisplayName();
             if (strategy.isApplicable(job)) {
                 try {

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

@@ -35,7 +35,7 @@
 import org.kohsuke.accmod.restrictions.NoExternalUse;
 
 /**
- * Run background build discarders on an individual job once a build is finalized
+ * Run build discarders on an individual job once a build is finalized
  */
 @Extension
 @Restricted(NoExternalUse.class)
@@ -46,6 +46,15 @@ public class GlobalBuildDiscarderListener extends RunListener<Run> {
     @Override
     public void onFinalized(Run run) {
         Job job = run.getParent();
-        BackgroundGlobalBuildDiscarder.processJob(new LogTaskListener(LOGGER, Level.FINE), job);
+        try {
+            // Job-level build discarder execution is unconditional.
+            job.logRotate();
+        } catch (Exception e) {
+            LOGGER.log(Level.WARNING, e, () -> "Failed to rotate log for " + run);
+        }
+        // Avoid calling Job.logRotate twice in case JobGlobalBuildDiscarderStrategy is configured globally.
+        BackgroundGlobalBuildDiscarder.processJob(new LogTaskListener(LOGGER, Level.FINE), job,
+                GlobalBuildDiscarderConfiguration.get().getConfiguredBuildDiscarders().stream()
+                        .filter(s -> !(s instanceof JobGlobalBuildDiscarderStrategy)));
     }
 }

core/src/main/resources/hudson/model/Run/delete.jelly

@@ -27,7 +27,7 @@ THE SOFTWARE.
 -->
 <?jelly escape-by-default='true'?>
 <j:jelly xmlns:j="jelly:core" xmlns:l="/lib/layout">
-  <j:if test="${!it.building and !it.keepLog}">
+  <j:if test="${!it.logUpdated and !it.keepLog}">
     <l:task href="${buildUrl.baseUrl}/confirmDelete" icon="icon-edit-delete icon-md" permission="${it.DELETE}" title="${%delete.build(it.displayName)}"/>
   </j:if>
 </j:jelly>

test/src/test/java/hudson/cli/DeleteBuildsCommandTest.java

@@ -32,21 +32,18 @@
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.hasSize;
 import static org.junit.Assert.assertNotNull;
-import static org.junit.Assume.assumeFalse;
 
-import hudson.Functions;
 import hudson.model.ExecutorTest;
 import hudson.model.FreeStyleProject;
 import hudson.model.Item;
 import hudson.model.Run;
 import hudson.model.labels.LabelAtom;
 import hudson.tasks.Shell;
-import java.io.IOException;
 import jenkins.model.Jenkins;
-import org.junit.AssumptionViolatedException;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
+import org.jvnet.hudson.test.Issue;
 import org.jvnet.hudson.test.JenkinsRule;
 
 /**
@@ -139,24 +136,18 @@ public class DeleteBuildsCommandTest {
         assertThat(result.stdout(), containsString("Deleted 0 builds"));
     }
 
-    @Test public void deleteBuildsShouldSuccessEvenTheBuildIsRunning() throws Exception {
-        assumeFalse("You can't delete files that are in use on Windows", Functions.isWindows());
+    @Issue("JENKINS-73835")
+    @Test public void deleteBuildsShouldFailIfTheBuildIsRunning() throws Exception {
         FreeStyleProject project = j.createFreeStyleProject("aProject");
         ExecutorTest.startBlockingBuild(project);
         assertThat(((FreeStyleProject) j.jenkins.getItem("aProject")).getBuilds(), hasSize(1));
 
         final CLICommandInvoker.Result result = command
                 .authorizedTo(Jenkins.READ, Item.READ, Run.DELETE)
                 .invokeWithArgs("aProject", "1");
-        assertThat(result, succeeded());
-        assertThat(result.stdout(), containsString("Deleted 1 builds"));
-        assertThat(((FreeStyleProject) j.jenkins.getItem("aProject")).getBuilds(), hasSize(0));
-        assertThat(project.isBuilding(), equalTo(false));
-        try {
-            project.delete();
-        } catch (IOException | InterruptedException x) {
-            throw new AssumptionViolatedException("Could not delete test project (race condition?)", x);
-        }
+        assertThat(result, failedWith(1));
+        assertThat(result, hasNoStandardOutput());
+        assertThat(result.stderr(), containsString("Unable to delete aProject #1 because it is still running"));
     }
 
     @Test public void deleteBuildsShouldSuccessEvenTheBuildIsStuckInTheQueue() throws Exception {

test/src/test/java/hudson/model/RunTest.java

@@ -30,6 +30,7 @@
 import static org.hamcrest.Matchers.not;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertThrows;
 import static org.junit.Assert.assertTrue;
 
 import hudson.ExtensionList;
@@ -62,6 +63,7 @@
 import org.junit.experimental.categories.Category;
 import org.jvnet.hudson.test.Issue;
 import org.jvnet.hudson.test.JenkinsRule;
+import org.jvnet.hudson.test.SleepBuilder;
 import org.jvnet.hudson.test.SmokeTest;
 import org.jvnet.hudson.test.TestExtension;
 import org.kohsuke.stapler.DataBoundConstructor;
@@ -128,6 +130,18 @@ public void onDeleted(Saveable o, XmlFile file) {
         }
     }
 
+    @Issue("JENKINS-73835")
+    @Test public void buildsMayNotBeDeletedWhileRunning() throws Exception {
+        var p = j.createFreeStyleProject();
+        p.getBuildersList().add(new SleepBuilder(999999));
+        var b = p.scheduleBuild2(0).waitForStart();
+        var ex = assertThrows(IOException.class, () -> b.delete());
+        assertThat(ex.getMessage(), containsString("Unable to delete " + b + " because it is still running"));
+        b.getExecutor().interrupt();
+        j.waitForCompletion(b);
+        b.delete(); // Works fine.
+    }
+
     @Issue("SECURITY-1902")
     @Test public void preventXssInBadgeTooltip() throws Exception {
         j.jenkins.setQuietPeriod(0);

test/src/test/java/hudson/tasks/LogRotatorTest.java

@@ -50,8 +50,10 @@
 import java.util.concurrent.TimeoutException;
 import java.util.logging.Level;
 import java.util.logging.Logger;
+import org.junit.ClassRule;
 import org.junit.Rule;
 import org.junit.Test;
+import org.jvnet.hudson.test.BuildWatcher;
 import org.jvnet.hudson.test.FailureBuilder;
 import org.jvnet.hudson.test.Issue;
 import org.jvnet.hudson.test.JenkinsRule;
@@ -62,6 +64,9 @@
  */
 public class LogRotatorTest {
 
+    @ClassRule
+    public static BuildWatcher watcher = new BuildWatcher();
+
     @Rule
     public JenkinsRule j = new JenkinsRule();
 
@@ -96,6 +101,17 @@ public void successVsFailureWithRemoveLastBuild() throws Exception {
         assertEquals(2, numberOf(project.getLastFailedBuild()));
     }
 
+    @Test
+    public void ableToDeleteCurrentBuild() throws Exception {
+        var p = j.createFreeStyleProject();
+        // Keep 0 builds, i.e. immediately delete builds as they complete.
+        LogRotator logRotator = new LogRotator(-1, 0, -1, -1);
+        logRotator.setRemoveLastBuild(true);
+        p.setBuildDiscarder(logRotator);
+        j.buildAndAssertStatus(Result.SUCCESS, p);
+        assertNull(p.getBuildByNumber(1));
+    }
+
     @Test
     @Issue("JENKINS-2417")
     public void stableVsUnstable() throws Exception {