Merge pull request #2439 from jglick/nextBuildNumber-JENKINS-27530

[JENKINS-27530] nextBuildNumber snafu after reload
jenkinsci · Jul 8, 2016 · 3ef1db5 · 3ef1db5
2 parents c528cdc + 12fd3e4
commit 3ef1db5
Show file tree

Hide file tree

Showing 9 changed files with 97 additions and 25 deletions.
diff --git 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<JobT extends Job<JobT, RunT>, RunT extends Run<JobT, RunT>>
         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<? extends Item> 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<Integer> foldersInt = Collections2.transform(Arrays.asList(folders), new Function<File, Integer>() {
-                        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
@@ -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<Executor, JobOffer> parked = new HashMap<Executor, JobOffer>();
@@ -2785,6 +2790,11 @@ public Snapshot(Set<WaitingItem> waitingList, List<BlockedItem> blockedProjects,
             this.buildables = new ArrayList<BuildableItem>(buildables);
             this.pendings = new ArrayList<BuildableItem>(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
@@ -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
@@ -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
@@ -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
@@ -25,13 +25,15 @@
 package jenkins.model.lazy;
 
 import hudson.Extension;
+import hudson.model.AbstractItem;
 import hudson.model.Item;
 import hudson.model.ItemGroup;
 import hudson.model.Job;
 import hudson.model.Queue;
 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<? extends Item> parent, String name) throws IOException {
         RunMap<RunT> _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<RunT> currentBuilds = this.builds;
         if (parent != null) {
             // are we overwriting what currently exist?
@@ -121,6 +129,7 @@ public void onLoad(ItemGroup<? extends Item> 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
@@ -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
@@ -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
@@ -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<Queue.Executable> 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.
      */