diff --git a/core/src/main/java/hudson/model/Job.java b/core/src/main/java/hudson/model/Job.java index 1a61359972c2..ee6cb07a98a9 100644 --- a/core/src/main/java/hudson/model/Job.java +++ b/core/src/main/java/hudson/model/Job.java @@ -23,8 +23,6 @@ */ package hudson.model; -import com.google.common.base.Function; -import com.google.common.collect.Collections2; import com.infradna.tool.bridge_method_injector.WithBridgeMethods; import hudson.BulkChange; @@ -126,6 +124,8 @@ public abstract class Job, RunT extends Run> extends AbstractItem implements ExtensionPoint, StaplerOverridable, ModelObjectWithChildren, OnMaster { + private static final Logger LOGGER = Logger.getLogger(Job.class.getName()); + /** * Next build number. Kept in a separate file because this is the only * information that gets updated often. This allows the rest of the @@ -206,24 +206,16 @@ public void onLoad(ItemGroup parent, String name) this.nextBuildNumber = Integer.parseInt(f.readTrim()); } } catch (NumberFormatException e) { - // try to infer the value of the next build number from the existing build records. See JENKINS-11563 - File[] folders = buildDir.listFiles(new FileFilter() { - public boolean accept(File file) { - return file.isDirectory() && file.getName().matches("[0-9]+"); - } - }); - - if (folders == null || folders.length == 0) { - this.nextBuildNumber = 1; + LOGGER.log(Level.WARNING, "Corruption in {0}: {1}", new Object[] {f, e}); + if (this instanceof LazyBuildMixIn.LazyLoadingJob) { + // allow LazyBuildMixIn.onLoad to fix it } else { - Collection foldersInt = Collections2.transform(Arrays.asList(folders), new Function() { - public Integer apply(File file) { - return Integer.parseInt(file.getName()); - } - }); - this.nextBuildNumber = Collections.max(foldersInt) + 1; + RunT lB = getLastBuild(); + synchronized (this) { + this.nextBuildNumber = lB != null ? lB.getNumber() + 1 : 1; + } + saveNextBuildNumber(); } - saveNextBuildNumber(); } } else { // From the old Hudson, or doCreateItem. Create this file now. @@ -1247,7 +1239,7 @@ public synchronized void doConfigSubmit(StaplerRequest req, FormApply.success(".").generateResponse(req, rsp, null); } } catch (JSONException e) { - Logger.getLogger(Job.class.getName()).log(Level.WARNING, "failed to parse " + json, e); + LOGGER.log(Level.WARNING, "failed to parse " + json, e); sendError(e, req, rsp); } } diff --git a/core/src/main/java/hudson/model/Queue.java b/core/src/main/java/hudson/model/Queue.java index e6eafc3ee33a..28bf6e1af59d 100644 --- a/core/src/main/java/hudson/model/Queue.java +++ b/core/src/main/java/hudson/model/Queue.java @@ -342,6 +342,11 @@ static class State { public void load() { lock.lock(); try { try { + // Clear items, for the benefit of reloading. + waitingList.clear(); + blockedProjects.clear(); + buildables.clear(); + pendings.clear(); // first try the old format File queueFile = getQueueFile(); if (queueFile.exists()) { @@ -1375,7 +1380,7 @@ public void maintain() { lock.lock(); try { try { - LOGGER.log(Level.FINE, "Queue maintenance started {0}", this); + LOGGER.log(Level.FINE, "Queue maintenance started on {0} with {1}", new Object[] {this, snapshot}); // The executors that are currently waiting for a job to run. Map parked = new HashMap(); @@ -2785,6 +2790,11 @@ public Snapshot(Set waitingList, List blockedProjects, this.buildables = new ArrayList(buildables); this.pendings = new ArrayList(pendings); } + + @Override + public String toString() { + return "Queue.Snapshot{waitingList=" + waitingList + ";blockedProjects=" + blockedProjects + ";buildables=" + buildables + ";pendings=" + pendings + "}"; + } } private static class LockedRunnable implements Runnable { diff --git a/core/src/main/java/hudson/model/RunMap.java b/core/src/main/java/hudson/model/RunMap.java index 3142dc764388..33322839cca5 100644 --- a/core/src/main/java/hudson/model/RunMap.java +++ b/core/src/main/java/hudson/model/RunMap.java @@ -185,7 +185,7 @@ public R put(R r) { // Defense against JENKINS-23152 and its ilk. File rootDir = r.getRootDir(); if (rootDir.isDirectory()) { - throw new IllegalStateException(rootDir + " already existed; will not overwrite with " + r); + throw new IllegalStateException("JENKINS-23152: " + rootDir + " already existed; will not overwrite with " + r); } if (!r.getClass().getName().equals("hudson.matrix.MatrixRun")) { // JENKINS-26739: grandfathered in proposeNewNumber(r.getNumber()); diff --git a/core/src/main/java/jenkins/model/Jenkins.java b/core/src/main/java/jenkins/model/Jenkins.java index 3c298d44f6df..61f22a4f9098 100644 --- a/core/src/main/java/jenkins/model/Jenkins.java +++ b/core/src/main/java/jenkins/model/Jenkins.java @@ -44,7 +44,6 @@ import hudson.Functions; import hudson.Launcher; import hudson.Launcher.LocalLauncher; -import hudson.LocalPluginManager; import hudson.Lookup; import hudson.Main; import hudson.Plugin; @@ -239,7 +238,6 @@ import org.kohsuke.accmod.restrictions.NoExternalUse; import org.kohsuke.args4j.Argument; import org.kohsuke.args4j.Option; -import org.kohsuke.stapler.DataBoundSetter; import org.kohsuke.stapler.HttpRedirect; import org.kohsuke.stapler.HttpResponse; import org.kohsuke.stapler.HttpResponses; @@ -311,6 +309,7 @@ import static hudson.Util.*; import static hudson.init.InitMilestone.*; +import hudson.init.Initializer; import hudson.util.LogTaskListener; import static java.util.logging.Level.*; import static javax.servlet.http.HttpServletResponse.*; @@ -3714,10 +3713,13 @@ public void run() { /** * Reloads the configuration synchronously. + * Beware that this calls neither {@link ItemListener#onLoaded} nor {@link Initializer}s. */ public void reload() throws IOException, InterruptedException, ReactorException { + queue.save(); executeReactor(null, loadTasks()); User.reload(); + queue.load(); servletContext.setAttribute("app", this); } diff --git a/core/src/main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java b/core/src/main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java index 25ac5f4dfcb3..46bd2be841f1 100644 --- a/core/src/main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java +++ b/core/src/main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java @@ -368,9 +368,17 @@ public R getByNumber(int n) { } } + /** + * @return the highest recorded build number, or 0 if there are none + */ + @Restricted(NoExternalUse.class) + public synchronized int maxNumberOnDisk() { + return numberOnDisk.max(); + } + protected final synchronized void proposeNewNumber(int number) throws IllegalStateException { - if (numberOnDisk.isInRange(numberOnDisk.ceil(number))) { - throw new IllegalStateException("cannot create a build with number " + number + " since that (or higher) is already in use among " + numberOnDisk); + if (number <= maxNumberOnDisk()) { + throw new IllegalStateException("JENKINS-27530: cannot create a build with number " + number + " since that (or higher) is already in use among " + numberOnDisk); } } diff --git a/core/src/main/java/jenkins/model/lazy/LazyBuildMixIn.java b/core/src/main/java/jenkins/model/lazy/LazyBuildMixIn.java index b206035c386d..6778edf7b224 100644 --- a/core/src/main/java/jenkins/model/lazy/LazyBuildMixIn.java +++ b/core/src/main/java/jenkins/model/lazy/LazyBuildMixIn.java @@ -25,6 +25,7 @@ package jenkins.model.lazy; import hudson.Extension; +import hudson.model.AbstractItem; import hudson.model.Item; import hudson.model.ItemGroup; import hudson.model.Job; @@ -32,6 +33,7 @@ import hudson.model.Run; import hudson.model.RunMap; import hudson.model.listeners.ItemListener; +import hudson.model.queue.SubTask; import hudson.widgets.BuildHistoryWidget; import hudson.widgets.HistoryWidget; import java.io.File; @@ -100,6 +102,12 @@ public final void onCreatedFromScratch() { @SuppressWarnings("unchecked") public void onLoad(ItemGroup parent, String name) throws IOException { RunMap _builds = createBuildRunMap(); + int max = _builds.maxNumberOnDisk(); + int next = asJob().getNextBuildNumber(); + if (next <= max) { + LOGGER.log(Level.WARNING, "JENKINS-27530: improper nextBuildNumber {0} detected in {1} with highest build number {2}; adjusting", new Object[] {next, asJob(), max}); + asJob().updateNextBuildNumber(max + 1); + } RunMap currentBuilds = this.builds; if (parent != null) { // are we overwriting what currently exist? @@ -121,6 +129,7 @@ public void onLoad(ItemGroup parent, String name) throws IOExcep if (r.isBuilding()) { // Do not use RunMap.put(Run): _builds.put(r.getNumber(), r); + LOGGER.log(Level.FINE, "keeping reloaded {0}", r); } } } diff --git a/core/src/main/java/jenkins/model/lazy/SortedIntList.java b/core/src/main/java/jenkins/model/lazy/SortedIntList.java index 27933fd47e44..24f0835f7a0c 100644 --- a/core/src/main/java/jenkins/model/lazy/SortedIntList.java +++ b/core/src/main/java/jenkins/model/lazy/SortedIntList.java @@ -83,6 +83,10 @@ public int size() { return size; } + public int max() { + return size > 0 ? data[size - 1] : 0; + } + @Override public boolean add(Integer i) { return add(i.intValue()); diff --git a/core/src/test/java/jenkins/model/lazy/SortedIntListTest.java b/core/src/test/java/jenkins/model/lazy/SortedIntListTest.java index 2146852a5f90..9748aeaa1881 100644 --- a/core/src/test/java/jenkins/model/lazy/SortedIntListTest.java +++ b/core/src/test/java/jenkins/model/lazy/SortedIntListTest.java @@ -34,4 +34,15 @@ public void testLower() { assertFalse(l.isInRange(3)); } + @Test public void max() { + SortedIntList l = new SortedIntList(5); + assertEquals(0, l.max()); + l.add(1); + assertEquals(1, l.max()); + l.add(5); + assertEquals(5, l.max()); + l.add(10); + assertEquals(10, l.max()); + } + } diff --git a/test/src/test/java/hudson/model/RunMapTest.java b/test/src/test/java/hudson/model/RunMapTest.java index 13d49dcd60ac..3d555f2691a2 100644 --- a/test/src/test/java/hudson/model/RunMapTest.java +++ b/test/src/test/java/hudson/model/RunMapTest.java @@ -1,5 +1,6 @@ package hudson.model; +import hudson.model.queue.QueueTaskFuture; import javax.xml.transform.Source; import javax.xml.transform.stream.StreamSource; import static org.junit.Assert.*; @@ -12,6 +13,7 @@ public class RunMapTest { @Rule public JenkinsRule r = new JenkinsRule(); + // TODO https://github.com/jenkinsci/jenkins/pull/2438: @Rule public LoggerRule logs = new LoggerRule(); /** * Makes sure that reloading the project while a build is in progress won't clobber that in-progress build. @@ -42,6 +44,40 @@ public class RunMapTest { assertSame(b2.getPreviousBuild(), b1); } + @Issue("JENKINS-27530") + @Test public void reloadWhileBuildIsInQueue() throws Exception { + //logs.record(Queue.class, Level.FINE); + FreeStyleProject p = r.createFreeStyleProject("p"); + p.getBuildersList().add(new SleepBuilder(9999999)); + r.jenkins.setNumExecutors(1); + assertEquals(1, p.scheduleBuild2(0).waitForStart().number); + p.scheduleBuild2(0); + // Note that the bug does not reproduce simply from p.doReload(), since in that case Job identity remains intact: + r.jenkins.reload(); + p = r.jenkins.getItemByFullName("p", FreeStyleProject.class); + FreeStyleBuild b1 = p.getLastBuild(); + assertEquals(1, b1.getNumber()); + /* Currently fails since Run.project is final. But anyway that is not the problem: + assertEquals(p, b1.getParent()); + */ + Queue.Item[] items = Queue.getInstance().getItems(); + assertEquals(1, items.length); + assertEquals(p, items[0].task); // the real issue: assignBuildNumber was being called on the wrong Job + QueueTaskFuture b2f = items[0].getFuture(); + b1.getExecutor().interrupt(); + r.assertBuildStatus(Result.ABORTED, r.waitForCompletion(b1)); + FreeStyleBuild b2 = (FreeStyleBuild) b2f.waitForStart(); + assertEquals(2, b2.getNumber()); + assertEquals(p, b2.getParent()); + b2.getExecutor().interrupt(); + r.assertBuildStatus(Result.ABORTED, r.waitForCompletion(b2)); + FreeStyleBuild b3 = p.scheduleBuild2(0).waitForStart(); + assertEquals(3, b3.getNumber()); + assertEquals(p, b3.getParent()); + b3.getExecutor().interrupt(); + r.assertBuildStatus(Result.ABORTED, r.waitForCompletion(b3)); + } + /** * Testing if the lazy loading can gracefully tolerate a RuntimeException during unmarshalling. */