[FIXED JENKINS-19920] Using selfReference to mean “none” from...

[FIXED JENKINS-19920] Using selfReference to mean “none” from nextBuild/previousBuild was just asking for trouble.
JENKINS-16194 and an analogous but opposite bug fixed more simply by introducing a separate NONE constant.
Also numberOnDisk did not get updated properly after a build was deleted, causing double loading of build records in some cases.

changelog.html

@@ -61,6 +61,9 @@ Upcoming changes</a>
   <li class=bug>
     Windows JDK installer should not install a public JRE.
     (<a href="https://issues.jenkins-ci.org/browse/JENKINS-8957">issue 8957</a>)
+  <li class='major bug'>
+    After deleting last build, next build of last build is zombie.
+    (<a href="https://issues.jenkins-ci.org/browse/JENKINS-19920">issue 19920</a>)
   <li class='rfe'>
     Split matrix authorization strategies into an independent plugin.
   <li class='rfe'>

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

@@ -162,10 +162,13 @@ public abstract class AbstractBuild<P extends AbstractProject<P,R>,R extends Abs
      * <p>
      * Unlike {@link Run}, {@link AbstractBuild}s do lazy-loading, so we don't use
      * {@link Run#previousBuild} and {@link Run#nextBuild}, and instead use these
-     * fields and point to {@link #selfReference} of adjacent builds.
+     * fields and point to {@link #selfReference} (or {@link #none}) of adjacent builds.
      */
     private volatile transient BuildReference<R> previousBuild, nextBuild;
 
+    @SuppressWarnings({"unchecked", "rawtypes"}) private static final BuildReference NONE = new BuildReference("NONE", null);
+    @SuppressWarnings("unchecked") private BuildReference<R> none() {return NONE;}
+
     /*package*/ final transient BuildReference<R> selfReference = new BuildReference<R>(getId(),_this());
 
 
@@ -193,10 +196,6 @@ public abstract class AbstractBuild<P extends AbstractProject<P,R>,R extends Abs
         if(nextBuild!=null) {
             AbstractBuild nb = nextBuild.get();
             if (nb!=null) {
-                // remove the oldest build
-                if (previousBuild == selfReference) 
-                    nb.previousBuild = nextBuild;
-                else 
                 nb.previousBuild = previousBuild;
             }
         }
@@ -223,13 +222,11 @@ public abstract class AbstractBuild<P extends AbstractProject<P,R>,R extends Abs
                     this.previousBuild = pb.selfReference;
                     return pb;
                 } else {
-                    // this indicates that we know there's no previous build
-                    // (as opposed to we don't know if/what our previous build is.
-                    this.previousBuild = selfReference;
+                    this.previousBuild = none();
                     return null;
                 }
             }
-            if (r==selfReference)
+            if (r==none())
                 return null;
 
             R referent = r.get();
@@ -253,13 +250,11 @@ public abstract class AbstractBuild<P extends AbstractProject<P,R>,R extends Abs
                     this.nextBuild = nb.selfReference;
                     return nb;
                 } else {
-                    // this indicates that we know there's no next build
-                    // (as opposed to we don't know if/what our next build is.
-                    this.nextBuild = selfReference;
+                    this.nextBuild = none();
                     return null;
                 }
             }
-            if (r==selfReference)
+            if (r==none())
                 return null;
 
             R referent = r.get();

core/src/main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java

@@ -723,7 +723,11 @@ public abstract class AbstractLazyLoadRunMap<R> extends AbstractMap<Integer,R> i
 
     public synchronized boolean removeValue(R run) {
         Index copy = copy();
-        copy.byNumber.remove(getNumberOf(run));
+        int n = getNumberOf(run);
+        copy.byNumber.remove(n);
+        SortedIntList a = new SortedIntList(numberOnDisk);
+        a.removeValue(n);
+        numberOnDisk = a;
         BuildReference<R> old = copy.byId.remove(getIdOf(run));
         this.index = copy;
 

test/src/test/groovy/hudson/model/AbstractBuildTest.groovy

@@ -38,6 +38,7 @@ import org.jvnet.hudson.test.UnstableBuilder
 import static org.junit.Assert.*
 import org.junit.Rule
 import org.junit.Test
+import org.jvnet.hudson.test.Bug
 
 public class AbstractBuildTest {
 
@@ -121,4 +122,18 @@ public class AbstractBuildTest {
         b = j.assertBuildStatus(Result.SUCCESS,p.scheduleBuild2(0).get())
         assertCulprits(b,["george"])
     }
+
+    @Bug(19920)
+    @Test void lastBuildNextBuild() {
+        FreeStyleProject p = j.createFreeStyleProject();
+        AbstractBuild b1 = j.assertBuildStatusSuccess(p.scheduleBuild2(0));
+        AbstractBuild b2 = j.assertBuildStatusSuccess(p.scheduleBuild2(0));
+        assertEquals(b2, p.getLastBuild());
+        b2.getNextBuild(); // force this to be initialized
+        b2.delete();
+        assertEquals(b1, p.getLastBuild());
+        b1 = p.getLastBuild();
+        assertEquals(null, b1.getNextBuild());
+    }
+
 }