Merge pull request #1566 from tfennelly/JENKINS-26445

Permanently valid Queue.Item.id

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

@@ -109,6 +109,7 @@ import jenkins.security.QueueItemAuthenticator;
 import jenkins.util.AtmostOneTaskExecutor;
 import org.acegisecurity.AccessDeniedException;
 import org.acegisecurity.Authentication;
+import org.jenkinsci.bytecode.AdaptField;
 import org.kohsuke.stapler.HttpResponse;
 import org.kohsuke.stapler.HttpResponses;
 import org.kohsuke.stapler.export.Exported;
@@ -191,11 +192,11 @@ public class Queue extends ResourceController implements Saveable {
     private final ItemList<BuildableItem> pendings = new ItemList<BuildableItem>();
 
     /**
-     * Items that left queue would stay here for a while to enable tracking via {@link Item#id}.
+     * Items that left queue would stay here for a while to enable tracking via {@link Item#getId()}.
      *
      * This map is forgetful, since we can't remember everything that executed in the past.
      */
-    private final Cache<Integer,LeftItem> leftItems = CacheBuilder.newBuilder().expireAfterWrite(5*60, TimeUnit.SECONDS).build();
+    private final Cache<Long,LeftItem> leftItems = CacheBuilder.newBuilder().expireAfterWrite(5*60, TimeUnit.SECONDS).build();
 
     private final CachedItemList itemsView = new CachedItemList();
 
@@ -342,6 +343,14 @@ public class Queue extends ResourceController implements Saveable {
         this.sorter = sorter;
     }
 
+    /**
+     * Simple queue state persistence object.
+     */
+    static class State {
+        public long counter;
+        public List<Item> items = new ArrayList<Item>();
+    }
+
     /**
      * Loads the queue contents that was {@link #save() saved}.
      */
@@ -366,18 +375,36 @@ public class Queue extends ResourceController implements Saveable {
             } else {
                 queueFile = getXMLQueueFile();
                 if (queueFile.exists()) {
-                    List list = (List) new XmlFile(XSTREAM, queueFile).read();
-                    int maxId = 0;
-                    for (Object o : list) {
+                    Object unmarshaledObj = new XmlFile(XSTREAM, queueFile).read();
+                    List items;
+
+                    if (unmarshaledObj instanceof State) {
+                        State state = (State) unmarshaledObj;
+                        items = state.items;
+                        WaitingItem.COUNTER.set(state.counter);
+                    } else {
+                        // backward compatibility - it's an old List queue.xml
+                        items = (List) unmarshaledObj;
+                        long maxId = 0;
+                        for (Object o : items) {
+                            if (o instanceof Item) {
+                                maxId = Math.max(maxId, ((Item)o).id);
+                            }
+                        }
+                        WaitingItem.COUNTER.set(maxId);
+                    }
+
+                    for (Object o : items) {
                         if (o instanceof Task) {
                             // backward compatibility
                             schedule((Task)o, 0);
                         } else if (o instanceof Item) {
                             Item item = (Item)o;
-                            if(item.task==null)
+
+                            if (item.task == null) {
                                 continue;   // botched persistence. throw this one away
+                            }
 
-                            maxId = Math.max(maxId, item.id);
                             if (item instanceof WaitingItem) {
                                 item.enter(this);
                             } else if (item instanceof BlockedItem) {
@@ -387,9 +414,8 @@ public class Queue extends ResourceController implements Saveable {
                             } else {
                                 throw new IllegalStateException("Unknown item type! " + item);
                             }
-                        } // this conveniently ignores null
+                        }
                     }
-                    WaitingItem.COUNTER.set(maxId);
 
                     // I just had an incident where all the executors are dead at AbstractProject._getRuns()
                     // because runs is null. Debugger revealed that this is caused by a MatrixConfiguration
@@ -413,16 +439,19 @@ public class Queue extends ResourceController implements Saveable {
     public synchronized void save() {
         if(BulkChange.contains(this))  return;
 
+        // write out the queue state we want to save
+        State state = new State();
+        state.counter = WaitingItem.COUNTER.longValue();
+
         // write out the tasks on the queue
-    	ArrayList<Queue.Item> items = new ArrayList<Queue.Item>();
-    	for (Item item: getItems()) {
+        for (Item item: getItems()) {
             if(item.task instanceof TransientTask)  continue;
-    	    items.add(item);
+	    state.items.add(item);
     	}
 
         try {
             XmlFile queueFile = new XmlFile(XSTREAM, getXMLQueueFile());
-            queueFile.write(items);
+            queueFile.write(state);
             SaveableListener.fireOnChange(this, queueFile);
         } catch (IOException e) {
             LOGGER.log(Level.WARNING, "Failed to write out the queue file " + getXMLQueueFile(), e);
@@ -508,7 +537,7 @@ public class Queue extends ResourceController implements Saveable {
      *      is that such {@link Item} only captures the state of the item at a particular moment,
      *      and by the time you inspect the object, some of its information can be already stale.
      *
-     *      That said, one can still look at {@link Queue.Item#future}, {@link Queue.Item#id}, etc.
+     *      That said, one can still look at {@link Queue.Item#future}, {@link Queue.Item#getId()}, etc.
      */
     public synchronized @Nonnull ScheduleResult schedule2(Task p, int quietPeriod, List<Action> actions) {
         // remove nulls
@@ -536,7 +565,7 @@ public class Queue extends ResourceController implements Saveable {
      *      is that such {@link Item} only captures the state of the item at a particular moment,
      *      and by the time you inspect the object, some of its information can be already stale.
      *
-     *      That said, one can still look at {@link WaitingItem#future}, {@link WaitingItem#id}, etc.
+     *      That said, one can still look at {@link WaitingItem#future}, {@link WaitingItem#getId()}, etc.
      */
     private synchronized @Nonnull ScheduleResult scheduleInternal(Task p, int quietPeriod, List<Action> actions) {
         Calendar due = new GregorianCalendar();
@@ -666,7 +695,7 @@ public class Queue extends ResourceController implements Saveable {
      * Called from {@code queue.jelly} and {@code entries.jelly}.
      */
     @RequirePOST
-    public HttpResponse doCancelItem(@QueryParameter int id) throws IOException, ServletException {
+    public HttpResponse doCancelItem(@QueryParameter long id) throws IOException, ServletException {
         Item item = getItem(id);
         if (item != null) {
             cancel(item);
@@ -723,7 +752,7 @@ public class Queue extends ResourceController implements Saveable {
         return itemsView.get();
     }
 
-    public synchronized Item getItem(int id) {
+    public synchronized Item getItem(long id) {
     	for (Item item: waitingList) if (item.id == id) return item;
     	for (Item item: blockedProjects) if (item.id == id) return item;
     	for (Item item: buildables) if (item.id == id) return item;
@@ -1480,8 +1509,24 @@ public class Queue extends ResourceController implements Saveable {
          * VM-wide unique ID that tracks the {@link Task} as it moves through different stages
          * in the queue (each represented by different subtypes of {@link Item}.
          */
+	private final long id;
+
         @Exported
-    	public final int id;
+        public long getId() {
+            return id;
+        }
+
+        @AdaptField(was=int.class, name="id")
+        @Deprecated
+        public int getIdLegacy() {
+            if (id > Integer.MAX_VALUE) {
+                throw new IllegalStateException("Sorry, you need to update any Plugins attempting to " +
+                        "assign 'Queue.Item.id' to an int value. 'Queue.Item.id' is now a long value and " +
+                        "has incremented to a value greater than Integer.MAX_VALUE (2^31 - 1).");
+            }
+            return (int) id;
+        }
+
 
 		/**
          * Project to be built.
@@ -1536,7 +1581,7 @@ public class Queue extends ResourceController implements Saveable {
         /**
          * Can be used to wait for the completion (either normal, abnormal, or cancellation) of the {@link Task}.
          * <p>
-         * Just like {@link #id}, the same object tracks various stages of the queue.
+         * Just like {@link #getId()}, the same object tracks various stages of the queue.
          */
         @WithBridgeMethods(Future.class)
         public QueueTaskFuture<Executable> getFuture() { return future; }
@@ -1595,7 +1640,7 @@ public class Queue extends ResourceController implements Saveable {
             return s.toString();
         }
 
-        protected Item(Task task, List<Action> actions, int id, FutureImpl future) {
+        protected Item(Task task, List<Action> actions, long id, FutureImpl future) {
             this.task = task;
             this.id = id;
             this.future = future;
@@ -1603,7 +1648,7 @@ public class Queue extends ResourceController implements Saveable {
             for (Action action: actions) addAction(action);
         }
 
-        protected Item(Task task, List<Action> actions, int id, FutureImpl future, long inQueueSince) {
+        protected Item(Task task, List<Action> actions, long id, FutureImpl future, long inQueueSince) {
             this.task = task;
             this.id = id;
             this.future = future;
@@ -1796,7 +1841,7 @@ public class Queue extends ResourceController implements Saveable {
      * {@link Item} in the {@link Queue#waitingList} stage.
      */
     public static final class WaitingItem extends Item implements Comparable<WaitingItem> {
-    	private static final AtomicInteger COUNTER = new AtomicInteger(0);
+	private static final AtomicLong COUNTER = new AtomicLong(0);
 
         /**
          * This item can be run after this time.
@@ -1809,11 +1854,21 @@ public class Queue extends ResourceController implements Saveable {
             this.timestamp = timestamp;
         }
 
+        static int getCurrentCounterValue() {
+            return COUNTER.intValue();
+        }
+
         public int compareTo(WaitingItem that) {
             int r = this.timestamp.getTime().compareTo(that.timestamp.getTime());
             if (r != 0) return r;
 
-            return this.id - that.id;
+            if (this.getId() < that.getId()) {
+                return -1;
+            } else if (this.getId() == that.getId()) {
+                return 0;
+            } else {
+                return 1;
+            }
         }
 
         public CauseOfBlockage getCauseOfBlockage() {
@@ -2081,7 +2136,7 @@ public class Queue extends ResourceController implements Saveable {
 
         @Override
         void enter(Queue q) {
-            q.leftItems.put(id,this);
+            q.leftItems.put(getId(),this);
             for (QueueListener ql : QueueListener.all()) {
                 try {
                     ql.onLeft(this);

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

@@ -137,6 +137,11 @@ import org.kohsuke.stapler.interceptor.RequirePOST;
 public abstract class Run <JobT extends Job<JobT,RunT>,RunT extends Run<JobT,RunT>>
         extends Actionable implements ExtensionPoint, Comparable<RunT>, AccessControlled, PersistenceRoot, DescriptorByNameOwner, OnMaster {
 
+    /**
+     * The original {@link Queue.Item#getId()} has not yet been mapped onto the {@link Run} instance.
+     */
+    public static final long QUEUE_ID_UNKNOWN = -1;
+
     protected transient final @Nonnull JobT project;
 
     /**
@@ -148,6 +153,11 @@ public abstract class Run <JobT extends Job<JobT,RunT>,RunT extends Run<JobT,Run
      */
     public transient /*final*/ int number;
 
+    /**
+     * The original Queue task ID from where this Run instance originated.
+     */
+    private long queueId;
+
     /**
      * Previous build. Can be null.
      * TODO JENKINS-22052 this is not actually implemented any more
@@ -402,6 +412,29 @@ public abstract class Run <JobT extends Job<JobT,RunT>,RunT extends Run<JobT,Run
         return this.number - that.number;
     }
 
+    /**
+     * Get the {@link Queue.Item#getId()} of the original queue item from where this Run instance
+     * originated.
+     * @return The queue item ID.
+     * @since TODO
+     */
+    @Exported
+    public long getQueueId() {
+        return queueId;
+    }
+
+    /**
+     * Set the queue item ID.
+     * <p/>
+     * Mapped from the {@link Queue.Item#getId()}.
+     * @param queueId The queue item ID.
+     * @since TODO
+     */
+    @Restricted(NoExternalUse.class)
+    public void setQueueId(long queueId) {
+        this.queueId = queueId;
+    }
+
     /**
      * Returns the build result.
      *

core/src/main/java/hudson/model/queue/WorkUnit.java

@@ -28,6 +28,7 @@ import hudson.model.Queue;
 import hudson.model.Queue.Executable;
 import hudson.model.Queue.Task;
 import javax.annotation.CheckForNull;
+import hudson.model.Run;
 import org.kohsuke.accmod.Restricted;
 import org.kohsuke.accmod.restrictions.NoExternalUse;
 import org.kohsuke.stapler.export.ExportedBean;
@@ -85,6 +86,9 @@ public final class WorkUnit {
     @Restricted(NoExternalUse.class)
     public void setExecutable(Executable executable) {
         this.executable = executable;
+        if (executable instanceof Run) {
+            ((Run) executable).setQueueId(context.item.getId());
+        }
     }
 
     /**

test/src/test/java/hudson/model/QueueTest.java

@@ -27,6 +27,7 @@ import com.gargoylesoftware.htmlunit.html.HtmlFileInput;
 import com.gargoylesoftware.htmlunit.html.HtmlForm;
 import com.gargoylesoftware.htmlunit.html.HtmlPage;
 import hudson.Launcher;
+import hudson.XmlFile;
 import hudson.matrix.AxisList;
 import hudson.matrix.LabelAxis;
 import hudson.matrix.MatrixBuild;
@@ -81,6 +82,8 @@ import org.apache.commons.fileupload.disk.DiskFileItemFactory;
 import org.apache.commons.fileupload.servlet.ServletFileUpload;
 import org.apache.commons.io.FileUtils;
 import static org.junit.Assert.*;
+
+import org.junit.Assert;
 import org.junit.Rule;
 import org.junit.Test;
 import org.jvnet.hudson.test.Issue;
@@ -88,6 +91,7 @@ import org.jvnet.hudson.test.JenkinsRule;
 import org.jvnet.hudson.test.MockQueueItemAuthenticator;
 import org.jvnet.hudson.test.SequenceLock;
 import org.jvnet.hudson.test.TestBuilder;
+import org.jvnet.hudson.test.recipes.LocalData;
 import org.mortbay.jetty.Server;
 import org.mortbay.jetty.bio.SocketConnector;
 import org.mortbay.jetty.servlet.ServletHandler;
@@ -116,24 +120,45 @@ public class QueueTest {
 
         System.out.println(FileUtils.readFileToString(new File(r.jenkins.getRootDir(), "queue.xml")));
 
-        assertEquals(1,q.getItems().length);
+        assertEquals(1, q.getItems().length);
         q.clear();
         assertEquals(0,q.getItems().length);
 
         // load the contents back
         q.load();
-        assertEquals(1,q.getItems().length);
+        assertEquals(1, q.getItems().length);
 
         // did it bind back to the same object?
         assertSame(q.getItems()[0].task,testProject);        
     }
 
+    /**
+     * Make sure the queue can be reconstructed from a List queue.xml.
+     * Prior to the Queue.State class, the Queue items were just persisted as a List.
+     */
+    @LocalData
+    @Test
+    public void recover_from_legacy_list() throws Exception {
+        Queue q = r.jenkins.getQueue();
+
+        // loaded the legacy queue.xml from test LocalData located in
+        // resources/hudson/model/QueueTest/recover_from_legacy_list.zip
+        assertEquals(1, q.getItems().length);
+
+        // The current counter should be the id from the item brought back
+        // from the persisted queue.xml.
+        assertEquals(3, Queue.WaitingItem.getCurrentCounterValue());
+    }
+
     /**
      * Can {@link Queue} successfully recover removal?
      */
      @Test public void persistence2() throws Exception {
         Queue q = r.jenkins.getQueue();
 
+        resetQueueState();
+        assertEquals(0, Queue.WaitingItem.getCurrentCounterValue());
+
         // prevent execution to push stuff into the queue
         r.jenkins.setNumExecutors(0);
         r.jenkins.setNodes(r.jenkins.getNodes());
@@ -144,7 +169,7 @@ public class QueueTest {
 
         System.out.println(FileUtils.readFileToString(new File(r.jenkins.getRootDir(), "queue.xml")));
 
-        assertEquals(1,q.getItems().length);
+        assertEquals(1, q.getItems().length);
         q.clear();
         assertEquals(0,q.getItems().length);
 
@@ -152,6 +177,27 @@ public class QueueTest {
         testProject.delete();
         q.load();
         assertEquals(0,q.getItems().length);
+
+        // The counter state should be maintained.
+        assertEquals(1, Queue.WaitingItem.getCurrentCounterValue());
+    }
+
+    /**
+     * Forces a reset of the private queue COUNTER.
+     * Could make changes to Queue to make that easier, but decided against that.
+     */
+    private void resetQueueState() throws IOException {
+        File queueFile = r.jenkins.getQueue().getXMLQueueFile();
+        XmlFile xmlFile = new XmlFile(Queue.XSTREAM, queueFile);
+        xmlFile.write(new Queue.State());
+        r.jenkins.getQueue().load();
+    }
+
+    @Test
+    public void queue_id_to_run_mapping() throws Exception {
+        FreeStyleProject testProject = r.createFreeStyleProject("test");
+        FreeStyleBuild build = r.assertBuildStatusSuccess(testProject.scheduleBuild2(0));
+        Assert.assertNotEquals(Run.QUEUE_ID_UNKNOWN, build.getQueueId());
     }
 
     /**
@@ -604,7 +650,7 @@ public class QueueTest {
         
         Queue.Item item = Queue.getInstance().getItem(p);
         assertNotNull(item);
-        Queue.getInstance().doCancelItem(item.id);
+        Queue.getInstance().doCancelItem(item.getId());
         assertNull(Queue.getInstance().getItem(p));
         
         try {
@@ -627,7 +673,7 @@ public class QueueTest {
             @Override
             public void run() {
                    try {
-                       Queue.getInstance().doCancelItem(item.id);
+                       Queue.getInstance().doCancelItem(item.getId());
                    } catch (IOException e) {
                        e.printStackTrace();
                    } catch (ServletException e) {