From d383906fbe2d29eb726cd7b3332aaf518923b3e6 Mon Sep 17 00:00:00 2001 From: Devin Nusbaum Date: Sat, 12 Oct 2024 08:06:18 -0400 Subject: [PATCH] [JENKINS-73835] Do not allow builds to be deleted while they are still running and ensure build discarders run after builds are fully complete (#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 (cherry picked from commit d34b17ee4b85787be56a2d6f32186e3839d5482d) --- core/src/main/java/hudson/model/Run.java | 9 +++----- .../main/java/hudson/tasks/LogRotator.java | 2 +- .../model/BackgroundGlobalBuildDiscarder.java | 13 +++++++++++- .../model/GlobalBuildDiscarderListener.java | 13 ++++++++++-- .../resources/hudson/model/Run/delete.jelly | 2 +- .../hudson/cli/DeleteBuildsCommandTest.java | 21 ++++++------------- test/src/test/java/hudson/model/RunTest.java | 14 +++++++++++++ .../java/hudson/tasks/LogRotatorTest.java | 16 ++++++++++++++ 8 files changed, 64 insertions(+), 26 deletions(-) diff --git a/core/src/main/java/hudson/model/Run.java b/core/src/main/java/hudson/model/Run.java index 37b09fd47d25..2b9fd5681028 100644 --- a/core/src/main/java/hudson/model/Run.java +++ b/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) { @@ -1883,12 +1886,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) { diff --git a/core/src/main/java/hudson/tasks/LogRotator.java b/core/src/main/java/hudson/tasks/LogRotator.java index ca95081b0385..0b47f035d809 100644 --- a/core/src/main/java/hudson/tasks/LogRotator.java +++ b/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; } diff --git a/core/src/main/java/jenkins/model/BackgroundGlobalBuildDiscarder.java b/core/src/main/java/jenkins/model/BackgroundGlobalBuildDiscarder.java index 1a42c0577a47..ad33643879f5 100644 --- a/core/src/main/java/jenkins/model/BackgroundGlobalBuildDiscarder.java +++ b/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 strategies) { + strategies.forEach(strategy -> { String displayName = strategy.getDescriptor().getDisplayName(); if (strategy.isApplicable(job)) { try { diff --git a/core/src/main/java/jenkins/model/GlobalBuildDiscarderListener.java b/core/src/main/java/jenkins/model/GlobalBuildDiscarderListener.java index 7ddea84c4241..6d2c58b44774 100644 --- a/core/src/main/java/jenkins/model/GlobalBuildDiscarderListener.java +++ b/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 { @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))); } } diff --git a/core/src/main/resources/hudson/model/Run/delete.jelly b/core/src/main/resources/hudson/model/Run/delete.jelly index 83e3cbd77e71..7443203f2650 100644 --- a/core/src/main/resources/hudson/model/Run/delete.jelly +++ b/core/src/main/resources/hudson/model/Run/delete.jelly @@ -27,7 +27,7 @@ THE SOFTWARE. --> - + diff --git a/test/src/test/java/hudson/cli/DeleteBuildsCommandTest.java b/test/src/test/java/hudson/cli/DeleteBuildsCommandTest.java index dfac64fc1b5a..5e2e917e9808 100644 --- a/test/src/test/java/hudson/cli/DeleteBuildsCommandTest.java +++ b/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,8 +136,8 @@ 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)); @@ -148,15 +145,9 @@ public class DeleteBuildsCommandTest { 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 { diff --git a/test/src/test/java/hudson/model/RunTest.java b/test/src/test/java/hudson/model/RunTest.java index b6a145857020..773fac5e395c 100644 --- a/test/src/test/java/hudson/model/RunTest.java +++ b/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.FilePath; @@ -59,6 +60,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; @@ -114,6 +116,18 @@ public class RunTest { assertTrue(Mgr.deleted.get()); } + @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); diff --git a/test/src/test/java/hudson/tasks/LogRotatorTest.java b/test/src/test/java/hudson/tasks/LogRotatorTest.java index b72e837dbd86..118a860147a1 100644 --- a/test/src/test/java/hudson/tasks/LogRotatorTest.java +++ b/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 {