Merge pull request #3 from mawinter69/JENKINS-66898

[fix JENKINS-66898] make the cache thread safe

Author: car-roll

src/main/java/org/jenkinsci/plugins/workflow/libs/LibraryAdder.java

@@ -43,8 +43,9 @@
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
-import java.util.Set;
 import java.util.TreeMap;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 import edu.umd.cs.findbugs.annotations.CheckForNull;
@@ -65,7 +66,13 @@
 @Extension public class LibraryAdder extends ClasspathAdder {
 
     private static final Logger LOGGER = Logger.getLogger(LibraryAdder.class.getName());
+    
+    private static ConcurrentHashMap<String, ReentrantReadWriteLock> cacheRetrieveLock = new ConcurrentHashMap<>();
 
+    static @NonNull ReentrantReadWriteLock getReadWriteLockFor(@NonNull String name) {
+        return cacheRetrieveLock.computeIfAbsent(name, s -> new ReentrantReadWriteLock(true));
+    }
+    
     @Override public List<Addition> add(CpsFlowExecution execution, List<String> libraries, HashMap<String, Boolean> changelogs) throws Exception {
         Queue.Executable executable = execution.getOwner().getExecutable();
         Run<?,?> build;
@@ -156,6 +163,36 @@
         }
     }
 
+    private enum CacheStatus {
+        VALID,
+        DOES_NOT_EXIST,
+        EXPIRED;
+    }
+    
+    private static CacheStatus getCacheStatus(@NonNull LibraryCachingConfiguration cachingConfiguration, @NonNull final FilePath versionCacheDir)
+          throws IOException, InterruptedException
+    {
+        if (cachingConfiguration.isRefreshEnabled()) {
+            final long cachingMilliseconds = cachingConfiguration.getRefreshTimeMilliseconds();
+
+            if(versionCacheDir.exists()) {
+                if ((versionCacheDir.lastModified() + cachingMilliseconds) > System.currentTimeMillis()) {
+                    return CacheStatus.VALID;
+                } else {
+                    return CacheStatus.EXPIRED;
+                }
+            } else {
+                return CacheStatus.DOES_NOT_EXIST;
+            }
+        } else {
+            if (versionCacheDir.exists()) {
+                return CacheStatus.VALID;
+            } else {
+                return CacheStatus.DOES_NOT_EXIST;
+            }
+        }
+    }
+    
     /** Retrieve library files. */
     static List<URL> retrieve(@NonNull LibraryRecord record, @NonNull LibraryRetriever retriever, @NonNull TaskListener listener, @NonNull Run<?,?> run, @NonNull CpsFlowExecution execution) throws Exception {
         String name = record.name;
@@ -165,42 +202,60 @@ static List<URL> retrieve(@NonNull LibraryRecord record, @NonNull LibraryRetriev
         FilePath libDir = new FilePath(execution.getOwner().getRootDir()).child("libs/" + record.getDirectoryName());
         Boolean shouldCache = cachingConfiguration != null;
         final FilePath versionCacheDir = new FilePath(LibraryCachingConfiguration.getGlobalLibrariesCacheDir(), record.getDirectoryName());
-        final FilePath retrieveLockFile = new FilePath(versionCacheDir, LibraryCachingConfiguration.RETRIEVE_LOCK_FILE);
+        ReentrantReadWriteLock retrieveLock = getReadWriteLockFor(record.getDirectoryName());
         final FilePath lastReadFile = new FilePath(versionCacheDir, LibraryCachingConfiguration.LAST_READ_FILE);
 
         if(shouldCache && cachingConfiguration.isExcluded(version)) {
             listener.getLogger().println("Library " + name + "@" + version + " is excluded from caching.");
             shouldCache = false;
         }
 
-        if(shouldCache && retrieveLockFile.exists()) {
-            listener.getLogger().println("Library " + name + "@" + version + " is currently being cached by another job, retrieving without cache.");
-            shouldCache = false;
-        }
-
         if(shouldCache) {
-            if (cachingConfiguration.isRefreshEnabled()) {
-                final long cachingMinutes = cachingConfiguration.getRefreshTimeMinutes();
-                final long cachingMilliseconds = cachingConfiguration.getRefreshTimeMilliseconds();
-
-                if(versionCacheDir.exists() && (versionCacheDir.lastModified() + cachingMilliseconds) < System.currentTimeMillis()) {
-                    listener.getLogger().println("Library " + name + "@" + version + " is due for a refresh after " + cachingMinutes + " minutes, clearing.");
-                    versionCacheDir.deleteRecursive();
+            retrieveLock.readLock().lockInterruptibly();
+            try {
+                CacheStatus cacheStatus = getCacheStatus(cachingConfiguration, versionCacheDir);
+                if (cacheStatus == CacheStatus.DOES_NOT_EXIST || cacheStatus == CacheStatus.EXPIRED) {
+                    retrieveLock.readLock().unlock();
+                    retrieveLock.writeLock().lockInterruptibly();
+                    try {
+                      boolean retrieve = false;
+                      switch (getCacheStatus(cachingConfiguration, versionCacheDir)) {
+                          case VALID: 
+                              listener.getLogger().println("Library " + name + "@" + version + " is cached. Copying from home."); 
+                                break;
+                          case DOES_NOT_EXIST:
+                              retrieve = true;
+                              break;
+                          case EXPIRED:
+                              long cachingMinutes = cachingConfiguration.getRefreshTimeMinutes();
+                              listener.getLogger().println("Library " + name + "@" + version + " is due for a refresh after " + cachingMinutes + " minutes, clearing.");
+                                if (versionCacheDir.exists()) {
+                                    versionCacheDir.deleteRecursive();
+                                    versionCacheDir.withSuffix("-name.txt").delete();
+                                }
+                                retrieve = true;
+                                break;
+                      }
+                            
+                        if (retrieve) {
+                            listener.getLogger().println("Caching library " + name + "@" + version);                            
+                            versionCacheDir.mkdirs();
+                            retriever.retrieve(name, version, changelog, versionCacheDir, run, listener);
+                        }
+                        retrieveLock.readLock().lock();
+                    } finally {
+                        retrieveLock.writeLock().unlock(); 
+                    }
+                } else {
+                    listener.getLogger().println("Library " + name + "@" + version + " is cached. Copying from home.");  
                 }
+  
+                lastReadFile.touch(System.currentTimeMillis());
+                versionCacheDir.withSuffix("-name.txt").write(name, "UTF-8");
+                versionCacheDir.copyRecursiveTo(libDir);
+            } finally {
+              retrieveLock.readLock().unlock();
             }
-
-            if(versionCacheDir.exists()) {
-                listener.getLogger().println("Library " + name + "@" + version + " is cached. Copying from home.");
-            } else {
-                listener.getLogger().println("Caching library " + name + "@" + version);
-                versionCacheDir.mkdirs();
-                retrieveLockFile.touch(System.currentTimeMillis());
-                retriever.retrieve(name, version, changelog, versionCacheDir, run, listener);
-                retrieveLockFile.delete();
-            }
-            lastReadFile.touch(System.currentTimeMillis());
-            versionCacheDir.withSuffix("-name.txt").write(name, "UTF-8");
-            versionCacheDir.copyRecursiveTo(libDir);
         } else {
             retriever.retrieve(name, version, changelog, libDir, run, listener);
         }

src/main/java/org/jenkinsci/plugins/workflow/libs/LibraryCachingCleanup.java

@@ -8,6 +8,7 @@
 
 import java.io.IOException;
 import java.util.concurrent.TimeUnit;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
 
 import jenkins.util.SystemProperties;
 
@@ -48,9 +49,16 @@ public LibraryCachingCleanup() {
     private boolean removeIfExpiredCacheDirectory(FilePath library) throws IOException, InterruptedException {
         final FilePath lastReadFile = new FilePath(library, LibraryCachingConfiguration.LAST_READ_FILE);
         if (lastReadFile.exists()) {
-            if (System.currentTimeMillis() - lastReadFile.lastModified() > TimeUnit.DAYS.toMillis(EXPIRE_AFTER_READ_DAYS)) {
-                library.deleteRecursive();
-                library.withSuffix("-name.txt").delete();
+            ReentrantReadWriteLock retrieveLock = LibraryAdder.getReadWriteLockFor(library.getName());
+            retrieveLock.writeLock().lockInterruptibly();
+            try {
+                if (System.currentTimeMillis() - lastReadFile.lastModified() > TimeUnit.DAYS.toMillis(EXPIRE_AFTER_READ_DAYS)) {
+                
+                    library.deleteRecursive();
+                    library.withSuffix("-name.txt").delete();
+                }
+            } finally {
+                retrieveLock.writeLock().unlock();
             }
             return true;
         }

src/main/java/org/jenkinsci/plugins/workflow/libs/LibraryCachingConfiguration.java

@@ -9,24 +9,30 @@
 import org.kohsuke.stapler.DataBoundConstructor;
 import org.kohsuke.stapler.QueryParameter;
 
-import java.io.File;
 import java.io.IOException;
 import java.io.InputStream;
 import java.nio.charset.StandardCharsets;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+import java.util.logging.Level;
+import java.util.logging.Logger;
+
 import org.apache.commons.io.IOUtils;
 import org.apache.commons.lang.StringUtils;
 
 public final class LibraryCachingConfiguration extends AbstractDescribableImpl<LibraryCachingConfiguration> {
+    
+    private static final Logger LOGGER = Logger.getLogger(LibraryCachingConfiguration.class.getName());
+    
     private int refreshTimeMinutes;
     private String excludedVersionsStr;
 
     private static final String VERSIONS_SEPARATOR = " ";
     public static final String GLOBAL_LIBRARIES_DIR = "global-libraries-cache";
     public static final String LAST_READ_FILE = "last_read";
-    public static final String RETRIEVE_LOCK_FILE = "retrieve.lock";
 
     @DataBoundConstructor public LibraryCachingConfiguration(int refreshTimeMinutes, String excludedVersionsStr) {
         this.refreshTimeMinutes = refreshTimeMinutes;
@@ -83,7 +89,7 @@ public static FilePath getGlobalLibrariesCacheDir() {
     }
 
     @Extension public static class DescriptorImpl extends Descriptor<LibraryCachingConfiguration> {
-        public FormValidation doClearCache(@QueryParameter String name) throws InterruptedException {
+        public FormValidation doClearCache(@QueryParameter String name, @QueryParameter boolean forceDelete) throws InterruptedException {
             Jenkins.get().checkPermission(Jenkins.ADMINISTER);
 
             try {
@@ -94,11 +100,27 @@ public FormValidation doClearCache(@QueryParameter String name) throws Interrupt
                         try (InputStream stream = libraryNamePath.read()) {
                             cacheName = IOUtils.toString(stream, StandardCharsets.UTF_8);
                         }
-                        if (libraryNamePath.readToString().equals(name)) {
+                        if (cacheName.equals(name)) {
                             FilePath libraryCachePath = LibraryCachingConfiguration.getGlobalLibrariesCacheDir()
                                     .child(libraryNamePath.getName().replace("-name.txt", ""));
-                            libraryCachePath.deleteRecursive();
-                            libraryNamePath.delete();
+                            if (forceDelete) {
+                                LOGGER.log(Level.FINER, "Force deleting cache for {0}", name);
+                                libraryCachePath.deleteRecursive();
+                                libraryNamePath.delete();
+                            } else {
+                                LOGGER.log(Level.FINER, "Safe deleting cache for {0}", name);
+                                ReentrantReadWriteLock retrieveLock = LibraryAdder.getReadWriteLockFor(libraryCachePath.getName());
+                                if (retrieveLock.writeLock().tryLock(10, TimeUnit.SECONDS)) {
+                                    try {
+                                        libraryCachePath.deleteRecursive();
+                                        libraryNamePath.delete();
+                                    } finally {
+                                        retrieveLock.writeLock().unlock();
+                                    }
+                                } else {
+                                    return FormValidation.error("The cache dir could not be deleted because it is currently being used by another thread. Please try again.");
+                                }
+                            }
                         }
                     }
                 }

src/main/resources/org/jenkinsci/plugins/workflow/libs/LibraryCachingConfiguration/config.jelly

@@ -32,6 +32,9 @@ THE SOFTWARE.
         <f:textbox />
     </f:entry>
     <j:if test="${h.hasPermission(app.ADMINISTER)}">
-        <f:validateButton title="${%Clear cache}" progress="${%Clearing...}" method="clearCache" with="name" />
+        <f:entry title="${%Force clear cache}" field="forceDelete">
+          <f:checkbox/>
+        </f:entry>
+        <f:validateButton title="${%Clear cache}" progress="${%Clearing...}" method="clearCache" with="name,forceDelete" />
     </j:if>
 </j:jelly>

src/main/resources/org/jenkinsci/plugins/workflow/libs/LibraryCachingConfiguration/help-forceDelete.html

@@ -0,0 +1,6 @@
+<div>
+    To avoid that clearing the cache interferes with running builds this process is normally guarded by locks.
+    Pressing the <i>Clear Cache</i> button will wait for a maximum of 10 seconds to get a lock.<br/>
+    By checking this option, the library will be deleted immediately without acquiring a lock. This can lead to build errors 
+    if a build is copying the library from the cache while it gets deleted. This risk is all the greater, the larger the library is.
+</div>

src/test/java/org/jenkinsci/plugins/workflow/libs/LibraryAdderTest.java

@@ -28,12 +28,14 @@
 import hudson.FilePath;
 import hudson.model.Job;
 import hudson.model.Result;
+import hudson.model.queue.QueueTaskFuture;
 import hudson.plugins.git.BranchSpec;
 import hudson.plugins.git.GitSCM;
 import hudson.plugins.git.UserRemoteConfig;
 import hudson.scm.ChangeLogSet;
 import hudson.scm.SubversionSCM;
 import hudson.slaves.WorkspaceList;
+import java.time.ZonedDateTime;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.Iterator;
@@ -473,6 +475,47 @@ public void correctLibraryDirectoryUsedWhenResumingOldBuild() throws Exception {
         r.assertLogContains("called Foo", b);
     }
 
+    @Issue("JENKINS-66898")
+    @Test
+    public void parallelBuildsDontInterfereWithExpiredCache() throws Throwable {
+        // Add a few files to the library so the deletion is not too fast
+        // Before fixing JENKINS-66898 this test was failing almost always
+        // with a build failure
+        sampleRepo.init();
+        sampleRepo.write("vars/foo.groovy", "def call() { echo 'foo' }");
+        sampleRepo.write("vars/bar.groovy", "def call() { echo 'bar' }");
+        sampleRepo.write("vars/foo2.groovy", "def call() { echo 'foo2' }");
+        sampleRepo.write("vars/foo3.groovy", "def call() { echo 'foo3' }");
+        sampleRepo.write("vars/foo4.groovy", "def call() { echo 'foo4' }");
+        sampleRepo.git("add", "vars");
+        sampleRepo.git("commit", "--message=init");
+        LibraryConfiguration config = new LibraryConfiguration("library",
+                new SCMSourceRetriever(new GitSCMSource(null, sampleRepo.toString(), "", "*", "", true)));
+        config.setDefaultVersion("master");
+        config.setImplicit(true);
+        config.setCachingConfiguration(new LibraryCachingConfiguration(30, null));
+        GlobalLibraries.get().getLibraries().add(config);
+        WorkflowJob p1 = r.createProject(WorkflowJob.class);
+        WorkflowJob p2 = r.createProject(WorkflowJob.class);
+        p1.setDefinition(new CpsFlowDefinition("foo()", true));
+        p2.setDefinition(new CpsFlowDefinition("foo()", true));
+        WorkflowRun b1 = r.buildAndAssertSuccess(p1);
+        LibrariesAction action = b1.getAction(LibrariesAction.class);
+        LibraryRecord record = action.getLibraries().get(0);
+        FilePath cache = LibraryCachingConfiguration.getGlobalLibrariesCacheDir().child(record.getDirectoryName());
+        //Expire the cache
+        long oldMillis = ZonedDateTime.now().minusMinutes(35).toInstant().toEpochMilli();
+        cache.touch(oldMillis);
+        QueueTaskFuture<WorkflowRun> f1 = p1.scheduleBuild2(0);
+        QueueTaskFuture<WorkflowRun> f2 = p2.scheduleBuild2(0);
+        r.assertBuildStatus(Result.SUCCESS, f1);
+        r.assertBuildStatus(Result.SUCCESS, f2);
+        // Disabling these 2 checks as they are flaky
+        // Occasionally the second job runs first and then build output doesn't match
+        // r.assertLogContains("is due for a refresh after", f1.get());
+        // r.assertLogContains("Library library@master is cached. Copying from home.", f2.get());
+    }
+
     @Issue("JENKINS-68544")
     @WithoutJenkins
     @Test public void className() {

src/test/java/org/jenkinsci/plugins/workflow/libs/LibraryCachingConfigurationTest.java

@@ -177,7 +177,7 @@ public void clearCache() throws Exception {
         assertThat(new File(cache.getRemote()), anExistingDirectory());
         assertThat(new File(cache.withSuffix("-name.txt").getRemote()), anExistingFile());
         // Clear the cache. TODO: Would be more realistic to set up security and use WebClient.
-        ExtensionList.lookupSingleton(LibraryCachingConfiguration.DescriptorImpl.class).doClearCache("library");
+        ExtensionList.lookupSingleton(LibraryCachingConfiguration.DescriptorImpl.class).doClearCache("library", false);
         assertThat(new File(cache.getRemote()), not(anExistingDirectory()));
         assertThat(new File(cache.withSuffix("-name.txt").getRemote()), not(anExistingFile()));
     }