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

…ld/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.
jenkinsci · Oct 7, 2013 · 7ca4dc4 · 7ca4dc4
1 parent 571de9c
commit 7ca4dc4
Show file tree

Hide file tree

Showing 4 changed files with 32 additions and 15 deletions.
diff --git a/changelog.html b/changelog.html
@@ -61,6 +61,9 @@
   <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'>

diff --git a/core/src/main/java/hudson/model/AbstractBuild.java b/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,11 +196,7 @@ void dropLinks() {
         if(nextBuild!=null) {
             AbstractBuild nb = nextBuild.get();
             if (nb!=null) {
-                // remove the oldest build
-                if (previousBuild == selfReference) 
-                    nb.previousBuild = nextBuild;
-                else 
-                    nb.previousBuild = previousBuild;
+                nb.previousBuild = previousBuild;
             }
         }
         if(previousBuild!=null) {
@@ -223,13 +222,11 @@ public R getPreviousBuild() {
                     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 R getNextBuild() {
                     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();

diff --git a/core/src/main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java b/core/src/main/java/jenkins/model/lazy/AbstractLazyLoadRunMap.java
@@ -723,7 +723,11 @@ protected BuildReference<R> createReference(R r) {
 
     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;
 

diff --git a/test/src/test/groovy/hudson/model/AbstractBuildTest.groovy b/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());
+    }
+
 }