[JENKINS-65829] Fix WorkspaceCleanupThread to consider suffixed workspaces even if original is inexistent

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

@@ -24,19 +24,28 @@
 
 package hudson.model;
 
+import static hudson.Util.fileToPath;
+
 import edu.umd.cs.findbugs.annotations.NonNull;
 import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import hudson.Extension;
 import hudson.ExtensionList;
 import hudson.FilePath;
 import hudson.Functions;
 import hudson.Util;
+import hudson.remoting.VirtualChannel;
+import hudson.slaves.WorkspaceList;
+import java.io.File;
+import java.io.FileFilter;
 import java.io.IOException;
+import java.io.Serializable;
+import java.nio.file.Path;
 import java.util.ArrayList;
 import java.util.Date;
 import java.util.List;
 import java.util.logging.Level;
 import java.util.logging.Logger;
+import jenkins.MasterToSlaveFileCallable;
 import jenkins.model.Jenkins;
 import jenkins.model.ModifiableTopLevelItemGroup;
 import jenkins.util.SystemProperties;
@@ -90,8 +99,7 @@
                 if (check) {
                     listener.getLogger().println("Deleting " + ws + " on " + node.getDisplayName());
                     try {
-                        ws.deleteSuffixesRecursive();
-                        ws.deleteRecursive();
+                        ws.act(new CleanupOldWorkspaces(retainForDays));
                     } catch (IOException | InterruptedException x) {
                         Functions.printStackTrace(x, listener.error("Failed to delete " + ws + " on " + node.getDisplayName()));
                     }
@@ -101,21 +109,6 @@
     }
 
     private boolean shouldBeDeleted(@NonNull TopLevelItem item, FilePath dir, @NonNull Node n) throws IOException, InterruptedException {
-        // TODO: the use of remoting is not optimal.
-        // One remoting can execute "exists", "lastModified", and "delete" all at once.
-        // (Could even invert master loop so that one FileCallable takes care of all known items.)
-        if (!dir.exists()) {
-            LOGGER.log(Level.FINE, "Directory {0} does not exist", dir);
-            return false;
-        }
-
-        // if younger than a month, keep it
-        long now = new Date().getTime();
-        if (dir.lastModified() + retainForDays * DAY > now) {
-            LOGGER.log(Level.FINE, "Directory {0} is only {1} old, so not deleting", new Object[] {dir, Util.getTimeSpanString(now - dir.lastModified())});
-            return false;
-        }
-
         // TODO could also be good to add checkbox that lets users configure a workspace to never be auto-cleaned.
 
         // TODO check instead for SCMTriggerItem:
@@ -143,11 +136,69 @@
                 return false;
             }
         }
-
-        LOGGER.log(Level.FINER, "Going to delete directory {0}", dir);
         return true;
     }
 
+    private static class CleanupOldWorkspaces extends MasterToSlaveFileCallable<Void> {
+
+        private final int retentionInDays;
+
+        public CleanupOldWorkspaces(int retentionInDays) {
+            this.retentionInDays = retentionInDays;
+        }
+
+        @Override
+        public Void invoke(File f, VirtualChannel channel) throws IOException, InterruptedException {
+            File[] workspaces = null;
+            File parentWs = f.getParentFile();
+            if (parentWs != null) {
+                workspaces = parentWs.listFiles(new ShouldBeDeletedFilter(this.retentionInDays, f.getName()));
+            }
+
+            if (workspaces != null) {
+                for (File workspace : workspaces) {
+                    LOGGER.log(Level.FINER, "Going to delete directory {0}", workspace);
+                    Util.deleteRecursive(fileToPath(workspace), Path::toFile);
+                }
+            }
+            return null;
+        }
+    }
+
+    private static class ShouldBeDeletedFilter implements FileFilter, Serializable {
+
+        private final int retentionInDays;
+
+        private final String workspaceBaseName;
+
+        public ShouldBeDeletedFilter(int retentionInDays, String workspaceBaseName) {
+            this.retentionInDays = retentionInDays;
+            this.workspaceBaseName = workspaceBaseName;
+        }
+
+        @Override
+        public boolean accept(File dir) {
+
+            if (!dir.isDirectory()) {
+                return false;
+            }
+
+            // if not the workspace or a workspace suffix
+            if (!dir.getName().equals(workspaceBaseName) && !dir.getName().startsWith(workspaceBaseName + WorkspaceList.COMBINATOR)) {
+                return false;
+            }
+
+            // if younger than a month, keep it
+            long now = new Date().getTime();
+            if (dir.lastModified() + this.retentionInDays * DAY > now) {
+                LOGGER.log(Level.FINE, "Directory {0} is only {1} old, so not deleting", new Object[] {dir, Util.getTimeSpanString(now - dir.lastModified())});
+                return false;
+            }
+
+            return true;
+        }
+    }
+
     private static final Logger LOGGER = Logger.getLogger(WorkspaceCleanupThread.class.getName());
 
     /**

test/src/test/java/hudson/model/WorkspaceCleanupThreadTest.java

@@ -40,6 +40,7 @@
 import java.util.concurrent.TimeUnit;
 import java.util.logging.Level;
 import jenkins.MasterToSlaveFileCallable;
+import jenkins.model.Jenkins;
 import org.junit.Assume;
 import org.junit.Rule;
 import org.junit.Test;
@@ -181,12 +182,28 @@ public void deleteTemporaryDirectory() throws Exception {
         FilePath ws = createOldWorkspaceOn(r.jenkins, p);
         FilePath tmp = WorkspaceList.tempDir(ws);
         tmp.child("stuff").write("content", null);
+        tmp.act(new Touch(0));
         createOldWorkspaceOn(r.createOnlineSlave(), p);
         performCleanup();
         assertFalse(ws.exists());
         assertFalse("temporary directory should be cleaned up as well", tmp.exists());
     }
 
+    @Issue("JENKINS-65829")
+    @Test
+    public void deleteSoleLibsDirectory() throws Exception {
+        FreeStyleProject p = r.createFreeStyleProject();
+        FilePath jobWs = Jenkins.get().getWorkspaceFor(p);
+        FilePath libsWs = jobWs.withSuffix(WorkspaceList.COMBINATOR + "libs");
+        libsWs.child("test-libs").write("content", null);
+        libsWs.act(new Touch(0));
+        assertFalse(jobWs.exists());
+        assertTrue(libsWs.exists());
+        performCleanup();
+        assertFalse(jobWs.exists());
+        assertFalse("libs directory should be cleaned up as well", libsWs.exists());
+    }
+
     private FilePath createOldWorkspaceOn(Node slave, FreeStyleProject p) throws Exception {
         p.setAssignedNode(slave);
         FreeStyleBuild b1 = r.buildAndAssertSuccess(p);
@@ -197,6 +214,13 @@ private FilePath createOldWorkspaceOn(Node slave, FreeStyleProject p) throws Exc
         return ws;
     }
 
+    private FilePath createOldLibsWorkspace(FreeStyleProject p) throws IOException, InterruptedException {
+        FilePath libsWs = Jenkins.get().getWorkspaceFor(p).withSuffix(WorkspaceList.COMBINATOR + "libs");
+        libsWs.child("test-libs").write("content", null);
+        libsWs.act(new Touch(0));
+        return libsWs;
+    }
+
     private void performCleanup() throws InterruptedException, IOException {
         new WorkspaceCleanupThread().execute(StreamTaskListener.fromStdout());
     }