[JENKINS-30424] Move build.save to the migration thread

src/main/java/org/jenkinsci/plugins/uniqueid/impl/Id.java

@@ -71,7 +71,7 @@ public void onAttached(Run<?, ?> r) {
         // NO-OP
     }
 
-    /** 
+    /**
      * Migrates the run away from using this Action.
      */
     public void onLoad(Run<?, ?> r) {

src/main/java/org/jenkinsci/plugins/uniqueid/impl/IdStoreMigratorV1ToV2.java

@@ -36,8 +36,8 @@ public class IdStoreMigratorV1ToV2 {
 
     private static Logger LOGGER = Logger.getLogger(IdStoreMigratorV1ToV2.class.getName());
 
-    private static final String MARKER_FILE_NAME = "unique-id-migration.txt";
-    
+    /* package */ static final String MARKER_FILE_NAME = "unique-id-migration.txt";
+
     /**
      * Migrates any IDs stored in Folder/Job/Run configuration 
      * @throws IOException
@@ -136,8 +136,16 @@ public void run() {
                         // touch something in the build just to force loading incase it gets more lazy in the future.
                         Object r = iterator.next();
                         if (r != null && r instanceof Run) {
-                            ((Run)r).getAllActions();
+                            Run run = ((Run) r);
+                            run.getAllActions();
+                            try {
+                                // Save the run here so its storage is updated (after being migrated in Id.onLoad(Run))
+                                run.save();
+                            } catch (IOException e) {
+                                LOGGER.log(Level.WARNING, "Can not save build {0} on job {1}", new Object[] { run.getNumber(), job.getName() });
+                            }
                         }
+
                         migratedBuilds++;
                     }
                 }

src/main/java/org/jenkinsci/plugins/uniqueid/impl/RunIdStore.java

@@ -4,10 +4,13 @@
 import hudson.model.Action;
 import hudson.model.Actionable;
 import hudson.model.Run;
+import jenkins.model.Jenkins;
 
+import java.io.File;
 import java.io.IOException;
 import java.util.Iterator;
 import java.util.List;
+import java.util.logging.Level;
 
 import org.kohsuke.accmod.Restricted;
 import org.kohsuke.accmod.restrictions.NoExternalUse;
@@ -30,12 +33,26 @@ public void remove(Run run) throws IOException {
         List<Id> ids = run.getActions(Id.class);
         if (!ids.isEmpty()) {
             actionList.removeAll(ids);
-            run.save();
+            saveIfNeeded(run);
         }
     }
 
     @Override
     public String get(Run thing) {
         return Id.getId((Actionable) thing);
     }
+
+    private void saveIfNeeded(Run run) throws IOException {
+        Jenkins jenkins = Jenkins.getInstance();
+        if (jenkins == null) {
+            throw new IllegalStateException("Jenkins is null, so it is impossible to migrate the ID");
+        }
+        File marker = new File(jenkins.getRootDir(), IdStoreMigratorV1ToV2.MARKER_FILE_NAME);
+        if (marker.exists()) {
+            // The background migration thread already finished (all builds have been already loaded at least once),
+            // so this is a belated load on a run that was not migrated in the main process (for some reason).
+            // Let's save it now.
+            run.save();
+        }
+    }
 }

src/test/java/org/jenkinsci/plugins/uniqueid/impl/IdStoreMigratorV1ToV2Test.java

@@ -43,7 +43,7 @@ public class IdStoreMigratorV1ToV2Test {
     public void testMigration() throws Exception {
         Jenkins jenkins = jenkinsRule.jenkins;
         assertThat("All jobs loaded correctly", jenkins.getAllItems(), hasSize(4));
-        
+
         Folder folderNoID = jenkins.getItem("folderNoID", jenkins, Folder.class);
         Folder folderWithID = jenkins.getItem("folderWithID", jenkins, Folder.class);
 
@@ -54,7 +54,7 @@ public void testMigration() throws Exception {
         assertThat(folderWithID, notNullValue());
         assertThat(jobNoID, notNullValue());
         assertThat(jobWithID, notNullValue());
-        
+
         checkID(folderNoID, null);
         checkID(folderWithID, "YzUxN2JiZTYtNGVhZS00NDQxLTg5NT");
 
@@ -66,10 +66,24 @@ public void testMigration() throws Exception {
         checkID(jobNoID.getBuildByNumber(2), null);
 
         // build 1 had no id so its generated on the fly from the parent
-        checkID(jobWithID.getBuildByNumber(1), "ZGQxMDNhYzUtMTJlOC00YTc4LTgzOT_" + jobWithID.getBuildByNumber(1).getId()); 
+        checkID(jobWithID.getBuildByNumber(1), "ZGQxMDNhYzUtMTJlOC00YTc4LTgzOT_" + jobWithID.getBuildByNumber(1).getId());
         checkID(jobWithID.getBuildByNumber(2), "NGQ0ODM2NjktZGM0OS00MjdkLWE3NT");
     }
-
+
+    @Test
+    @Issue("JENKINS-30424")
+    @LocalData
+    public void testMigrationLongLoadingBuilds() throws Exception {
+        Jenkins jenkins = jenkinsRule.jenkins;
+        assertThat("All jobs loaded correctly", jenkins.getAllItems(), hasSize(2));
+
+        Folder folderWithID = jenkins.getItem("folderWithID", jenkins, Folder.class);
+        Job jobWithID = jenkins.getItem("jobWithID", (ItemGroup)folderWithID, Job.class);
+        // Build #2 has a LongLoadingAction which simulates an action that blocks in the onLoad method
+        // This kind on delays causes issues on startup with version 2.1.0 (see issue linked to this test)
+        checkID(jobWithID.getBuildByNumber(2), "NGQ0ODM2NjktZGM0OS00MjdkLWE3NT");
+    }
+
     @Test
     @Issue("JENKINS-28883")
     public void migrateUnsupportedType() throws Exception {

src/test/java/org/jenkinsci/plugins/uniqueid/impl/LongLoadingAction.java

@@ -0,0 +1,31 @@
+package org.jenkinsci.plugins.uniqueid.impl;
+
+import hudson.model.Run;
+import jenkins.model.RunAction2;
+
+public class LongLoadingAction implements RunAction2 {
+
+    public String getIconFileName() {
+        return null;
+    }
+
+    public String getDisplayName() {
+        return null;
+    }
+
+    public String getUrlName() {
+        return null;
+    }
+
+    public void onAttached(Run<?, ?> r) {
+    }
+
+    public void onLoad(Run<?, ?> r) {
+        try {
+            Thread.sleep(1000);
+        } catch (InterruptedException e) {
+            e.printStackTrace();
+        }
+    }
+
+}

src/test/java/org/jenkinsci/plugins/uniqueid/impl/LongLoadingBuild.java

@@ -0,0 +1,37 @@
+package org.jenkinsci.plugins.uniqueid.impl;
+
+import java.io.File;
+import java.io.IOException;
+
+import hudson.model.Build;
+
+public class LongLoadingBuild extends Build<LongLoadingProject, LongLoadingBuild> {
+
+    private String check = null;
+
+    public LongLoadingBuild(LongLoadingProject project) throws IOException {
+        super(project);
+    }
+
+    public LongLoadingBuild(LongLoadingProject project, File buildDir) throws IOException {
+        super(project, buildDir);
+    }
+
+    @Override
+    public synchronized void save() throws IOException {
+        // Will produce a null pointer exception if called before onLoad finished.
+        check.toString();
+        super.save();
+    }
+
+    @Override
+    protected void onLoad() {
+        super.onLoad();
+        check = "Checked!";
+    }
+
+    @Override
+    public void run() {
+    }
+
+}

src/test/java/org/jenkinsci/plugins/uniqueid/impl/LongLoadingProject.java

@@ -0,0 +1,36 @@
+package org.jenkinsci.plugins.uniqueid.impl;
+
+import org.jvnet.hudson.test.TestExtension;
+import hudson.model.ItemGroup;
+import hudson.model.Messages;
+import hudson.model.Project;
+import hudson.model.TopLevelItem;
+import hudson.model.TopLevelItemDescriptor;
+import jenkins.model.Jenkins;
+
+public class LongLoadingProject extends Project<LongLoadingProject, LongLoadingBuild> implements TopLevelItem {
+
+    public LongLoadingProject(ItemGroup parent, String name) {
+        super(parent, name);
+    }
+
+    @Override
+    protected Class<LongLoadingBuild> getBuildClass() {
+        return LongLoadingBuild.class;
+    }
+
+    @TestExtension
+    public static final class DescriptorImpl extends TopLevelItemDescriptor {
+        public String getDisplayName() {
+            return Messages.FreeStyleProject_DisplayName();
+        }
+
+        public LongLoadingProject newInstance(ItemGroup parent, String name) {
+            return new LongLoadingProject(parent,name);
+        }
+    }
+
+    public TopLevelItemDescriptor getDescriptor() {
+        return (DescriptorImpl) Jenkins.getInstance().getDescriptorOrDie(LongLoadingProject.class);
+    }
+}