#3225 - Fixed issue with PackageGarbageCollector not identifying all … (

#3247) * #3225 - Fixed issue with PackageGarbageCollector not identifying all packages to clean up properly
Adobe-Consulting-Services · Jan 22, 2024 · fed615b · fed615b
1 parent b5a6918
commit fed615b
Show file tree

Hide file tree

Showing 4 changed files with 53 additions and 40 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -10,6 +10,10 @@ The format is based on [Keep a Changelog](http://keepachangelog.com)
 
 ## Unreleased ([details][unreleased changes details])
 
+### Fix
+
+- #3246 - PackageGarbageCollector is not cleaning up all packages since v6.3.4 (##3225)
+
 ## 6.3.4 - 2023-01-17
 
 - #3223 - Project with class extending WCMUsePojo leads to build error: cannot access aQute.bnd.annotation.ConsumerType
@@ -22,7 +26,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com)
 - #3162 - Renovator MCP: ensure old source path is removed
 - #3205 - HttpClientFactory: Expose a method to customize the underlying HttpClient
 - #3209 - WARN org.apache.sling.models.impl.ModelAdapterFactory - Cannot provide default for java.util.List<java.lang.String>
-- #3197 - Encrypt user credentials in ACS Content Sync   
+- #3197 - Encrypt user credentials in ACS Content Sync
 - #3196 - Content Sync: prevent exception when creating parent nodes
 - #3194 - Redirect Manager: Ignore Case value is not persisting
 
@@ -43,18 +47,20 @@ The format is based on [Keep a Changelog](http://keepachangelog.com)
 ## Removed
 
 - #3183 - Removed .wrap package including JackrabbitSessionIWrap and related classes which is no longer supported in Cloud Manager pipelines.
-  
+
 ## 6.1.0 - 2023-09-08
 
 ## Added
 
 - #3159 - Add PageProperty annotation for Sling Models
 - #3170 - Added a new MCP tool to bulk tag AEM content pages via an Excel file input.
+
 ## Fixed
 
 - #3147 - Fixed setting initial content-type when importing CFs from a spreadsheet
 - #3040 - Fixed bug where namespaced multi-fields would have the namespace 2 times
-- #3140 - Fixed issue where malformed MCP process nodes can cause a NPE that breaks the entire MPC reporting UI. Now displays more friendly values in UI to help remove the invalid nodes.
+- #3140 - Fixed issue where malformed MCP process nodes can cause a NPE that breaks the entire MPC reporting UI. Now
+  displays more friendly values in UI to help remove the invalid nodes.
 - #3150 - Support for case-insensitive redirect rules ( [NC] flag equivalent of apache)
 - #3138 - Re-arrange action removes data from redirect node
 

diff --git a/...in/java/com/adobe/acs/commons/packagegarbagecollector/PackageGarbageCollectionConfig.java b/...in/java/com/adobe/acs/commons/packagegarbagecollector/PackageGarbageCollectionConfig.java
@@ -22,20 +22,20 @@
 import org.osgi.service.metatype.annotations.AttributeDefinition;
 import org.osgi.service.metatype.annotations.ObjectClassDefinition;
 
-@ObjectClassDefinition(name = "ACS Commons - Package Garbage Collection Configuration", description = "Used to config a package garbage collection job")
+@ObjectClassDefinition(name = "ACS Commons - Package Garbage Collection Configuration", description = "Used to config a package garbage collection job.")
 public @interface PackageGarbageCollectionConfig {
-    @AttributeDefinition(name = "Schedule", description = "Cron expression detailing when the garbage collection is run. Default runs at 02:30 every day")
+    @AttributeDefinition(name = "Schedule", description = "Cron expression detailing when the garbage collection is run. Default runs at 02:30 every day.")
     String scheduler() default "0 30 2 ? * * *";
 
-    @AttributeDefinition(name = "Package Group Name", description = "The group name of the packages to remove")
+    @AttributeDefinition(name = "Package group name", description = "The group name of the packages to remove.")
     String groupName() default "";
 
-    @AttributeDefinition(name = "Max age of package", description = "Packages older than this (in days) will be removed. Default is 60 days")
+    @AttributeDefinition(name = "Max upload age of package", description = "Packages uploaded more than the given amount of days ago will be removed. Default is 60 days.")
     int maxAgeInDays() default 60;
 
-    @AttributeDefinition(name = "Remove not installed packages", description = "Remove packages that are not installed (or have been uninstalled) and  the created date is older than maxAgeInDays. Default is false.")
+    @AttributeDefinition(name = "Remove not installed packages", description = "Remove packages that are not installed (or have been uninstalled) and the upload date is older than maxAgeInDays. Default is false.")
     boolean removeNotInstalledPackages() default false;
 
     @AttributeDefinition(name = "webconsole.configurationFactory.nameHint")
-    String webconsole_configurationFactory_nameHint() default "Package Garbage Collection - Clear packages in {groupName} older than {maxAgeInDays} days using the schedule [{scheduler}]";
+    String webconsole_configurationFactory_nameHint() default "Package Garbage Collection - Clear packages in {groupName} uploaded older than {maxAgeInDays} days using the schedule [{scheduler}]";
 }
diff --git a/.../main/java/com/adobe/acs/commons/packagegarbagecollector/PackageGarbageCollectionJob.java b/.../main/java/com/adobe/acs/commons/packagegarbagecollector/PackageGarbageCollectionJob.java
@@ -96,7 +96,7 @@ public JobResult process(Job job) {
                     }
                     String packageDescription = getPackageDescription(definition);
                     LOG.info("Processing package {}", packageDescription);
-    
+
                     if (isPackageOldEnough(definition, maxAgeInDays)) {
                         if (removeNotInstalledPackages && !isInstalled(definition)) {
                             packageManager.remove(jcrPackage);
@@ -126,6 +126,7 @@ public JobResult process(Job job) {
     }
 
     private boolean isInstalled(JcrPackageDefinition pkgDefinition) {
+        // lastUnpacked is when the package was installed (aka unpacked) to this AEM/JCR.
         return pkgDefinition.getLastUnpacked() != null;
     }
 
@@ -139,7 +140,7 @@ protected UncheckedRepositoryException(RepositoryException e) {
         /**
          * Returns the cause of this exception.
          *
-         * @return  the {@code RepositoryException} which is the cause of this exception.
+         * @return the {@code RepositoryException} which is the cause of this exception.
          */
         @Override
         public RepositoryException getCause() {
@@ -151,22 +152,22 @@ public RepositoryException getCause() {
     private boolean isLatestInstalled(JcrPackageDefinition referencePkgDefinition, Stream<JcrPackage> installedPackages) throws RepositoryException {
         try {
             Optional<JcrPackageDefinition> lastInstalledPckDefinitionOptional = installedPackages
-               .map(p -> {
-                    try {
-                        return p.getDefinition();
-                    } catch (RepositoryException e) {
-                        String pckPath;
+                    .map(p -> {
                         try {
-                            pckPath = p.getNode().getPath();
-                        } catch (RepositoryException nestedException) {
-                            pckPath = "Unknown";
+                            return p.getDefinition();
+                        } catch (RepositoryException e) {
+                            String pckPath;
+                            try {
+                                pckPath = p.getNode().getPath();
+                            } catch (RepositoryException nestedException) {
+                                pckPath = "Unknown";
+                            }
+                            throw new UncheckedRepositoryException(new RepositoryException("Cannot read package definition of package " + pckPath, e));
                         }
-                        throw new UncheckedRepositoryException(new RepositoryException("Cannot read package definition of package " + pckPath, e));
-                    }
-                })
-                .filter(def -> isSameNameAndGroup(referencePkgDefinition.getId(), def.getId()))
-                .filter(def -> def.getLastUnpacked() != null)
-                .max(Comparator.comparing(def -> def.getLastUnpacked()));
+                    })
+                    .filter(def -> isSameNameAndGroup(referencePkgDefinition.getId(), def.getId()))
+                    .filter(def -> def.getLastUnpacked() != null)
+                    .max(Comparator.comparing(def -> def.getLastUnpacked()));
 
             if (lastInstalledPckDefinitionOptional.isPresent()) {
                 return lastInstalledPckDefinitionOptional.get().getId().equals(referencePkgDefinition.getId());
@@ -177,38 +178,43 @@ private boolean isLatestInstalled(JcrPackageDefinition referencePkgDefinition, S
         }
     }
 
-    public static boolean isSameNameAndGroup(PackageId thisPackageId, PackageId otherPackageId){
-           return otherPackageId.getGroup().equals(thisPackageId.getGroup())
-                        && otherPackageId.getName().equals(thisPackageId.getName());
+    public static boolean isSameNameAndGroup(PackageId thisPackageId, PackageId otherPackageId) {
+        return otherPackageId.getGroup().equals(thisPackageId.getGroup())
+                && otherPackageId.getName().equals(thisPackageId.getName());
     }
 
+
     private boolean isPackageOldEnough(JcrPackageDefinition pkgDefinition, Integer maxAgeInDays) throws RepositoryException, IOException {
         Period maxAge = Period.ofDays(maxAgeInDays);
         LocalDate oldestAge = LocalDate.now().minus(maxAge);
-        final Calendar packageCreatedAtCalendar;
+        // lastUnwrapped is when the package was UPLOADED to this AEM/JCR, lastUnpacked is what it was last INSTALLED!
+        Calendar packageUploadedDate;
 
         try {
-            packageCreatedAtCalendar = pkgDefinition.getCreated();
-            if (packageCreatedAtCalendar == null) {
+            // getLastUnwrapped() is when the package was introduced (aka uploaded) to this AEM/JCR.
+            // getCreated() is when the package was created (aka built) by the package manager; which could be years ago.
+            packageUploadedDate = pkgDefinition.getLastUnwrapped();
+
+            if (packageUploadedDate == null) {
                 // This should not happen, but if it does, we don't want to delete the package.
-                LOG.warn("Package [ {} ] has no created date, assuming it's NOT old enough", pkgDefinition.getNode().getPath());
+                LOG.warn("Package [ {} ] has no lastUnwrapped (uploaded) date, assuming it's NOT old enough", pkgDefinition.getNode().getPath());
                 return false;
             }
         } catch (RepositoryException e) {
-            LOG.error("Unable to get created date for package [ {} ]", pkgDefinition.getNode().getPath(), e);
+            LOG.error("Unable to get lastUnwrapped (uploaded) date for package [ {} ]", pkgDefinition.getNode().getPath(), e);
             return false;
         }
 
-        LocalDate packageCreatedAt = LocalDateTime.ofInstant(
-                packageCreatedAtCalendar.toInstant(),
-                packageCreatedAtCalendar.getTimeZone().toZoneId()).toLocalDate();
+        LocalDate packageUploadedAt = LocalDateTime.ofInstant(
+                packageUploadedDate.toInstant(),
+                packageUploadedDate.getTimeZone().toZoneId()).toLocalDate();
         String packageDescription = getPackageDescription(pkgDefinition);
 
         if (LOG.isDebugEnabled()) {
-            LOG.debug("Checking if package is old enough: Name: {}, Created At: {}, Oldest Age: {}",
-                    packageDescription, packageCreatedAt.format(LOCALIZED_DATE_FORMATTER), oldestAge.format(LOCALIZED_DATE_FORMATTER));
+            LOG.debug("Checking if package is old enough: Name: {}, Uploaded at: {}, Oldest age: {}",
+                    packageDescription, packageUploadedAt.format(LOCALIZED_DATE_FORMATTER), oldestAge.format(LOCALIZED_DATE_FORMATTER));
         }
-        return !packageCreatedAt.isAfter(oldestAge);
+        return !packageUploadedAt.isAfter(oldestAge);
     }
 
     private String getPackageDescription(JcrPackageDefinition definition) throws RepositoryException {

diff --git a/...t/java/com/adobe/acs/commons/packagegarbagecollector/PackageGarbageCollectionJobTest.java b/...t/java/com/adobe/acs/commons/packagegarbagecollector/PackageGarbageCollectionJobTest.java
@@ -133,7 +133,8 @@ JcrPackage mockPackage(Integer daysAgo, Integer lastUnpackedDaysAgo, String pack
         when(packageNode.getPath()).thenReturn("/etc/packages/" + packageName);
         JcrPackageDefinition definition = mock(JcrPackageDefinition.class);
         when(definition.getLastUnpacked()).thenReturn(getDate(lastUnpackedDaysAgo));
-        when(definition.getCreated()).thenReturn(getDate(daysAgo));
+        when(definition.getLastUnwrapped()).thenReturn(getDate(daysAgo));
+        when(definition.getCreated()).thenReturn(null);
         when(definition.getNode()).thenReturn(packageNode);
         PackageId pid = mock(PackageId.class);
         when(pid.getName()).thenReturn(name);