Improve artifact binary cleanup - only after commit

hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/JpaRolloutExecutor.java

@@ -476,9 +476,8 @@ private void executeRunningGroups(final JpaRollout rollout, final List<JpaRollou
     }
 
     private void updateTotalTargetCount(final JpaRolloutGroup rolloutGroup, final long countTargetsOfRolloutGroup) {
-        final JpaRollout jpaRollout = (JpaRollout) rolloutGroup.getRollout();
-        final long updatedTargetCount = jpaRollout.getTotalTargets()
-                - (rolloutGroup.getTotalTargets() - countTargetsOfRolloutGroup);
+        final JpaRollout jpaRollout = rolloutGroup.getRollout();
+        final long updatedTargetCount = jpaRollout.getTotalTargets() - (rolloutGroup.getTotalTargets() - countTargetsOfRolloutGroup);
         jpaRollout.setTotalTargets(updatedTargetCount);
         rolloutGroup.setTotalTargets((int) countTargetsOfRolloutGroup);
         rolloutRepository.save(jpaRollout);
@@ -574,12 +573,8 @@ private void startFirstRolloutGroup(final Rollout rollout) {
 
     private boolean ensureAllGroupsAreScheduled(final Rollout rollout) {
         final JpaRollout jpaRollout = (JpaRollout) rollout;
-
-        final List<JpaRolloutGroup> groupsToBeScheduled = rolloutGroupRepository.findByRolloutAndStatus(rollout,
-                RolloutGroupStatus.READY);
-        final long scheduledGroups = groupsToBeScheduled.stream()
-                .filter(group -> scheduleRolloutGroup(jpaRollout, group)).count();
-
+        final List<JpaRolloutGroup> groupsToBeScheduled = rolloutGroupRepository.findByRolloutAndStatus(rollout, RolloutGroupStatus.READY);
+        final long scheduledGroups = groupsToBeScheduled.stream().filter(group -> scheduleRolloutGroup(jpaRollout, group)).count();
         return scheduledGroups == groupsToBeScheduled.size();
     }
 
@@ -630,8 +625,8 @@ private JpaRolloutGroup fillRolloutGroupWithTargets(final JpaRollout rollout, fi
             do {
                 // Add up to TRANSACTION_TARGETS of the left targets
                 // In case a TransactionException is thrown this loop aborts
-                final long assigned = assignTargetsToGroupInNewTransaction(rollout, group, groupTargetFilter,
-                        Math.min(TRANSACTION_TARGETS, targetsLeftToAdd));
+                final long assigned = assignTargetsToGroupInNewTransaction(
+                        rollout, group, groupTargetFilter, Math.min(TRANSACTION_TARGETS, targetsLeftToAdd));
                 if (assigned == 0) {
                     break; // percent > 100 or some could have disappeared
                 } else {

hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/RepositoryApplicationConfiguration.java

@@ -357,8 +357,8 @@ RolloutGroupEvaluationManager evaluationManager(
             final List<RolloutGroupConditionEvaluator<RolloutGroup.RolloutGroupSuccessCondition>> successConditionEvaluators,
             final List<RolloutGroupActionEvaluator<RolloutGroup.RolloutGroupErrorAction>> errorActionEvaluators,
             final List<RolloutGroupActionEvaluator<RolloutGroup.RolloutGroupSuccessAction>> successActionEvaluators) {
-        return new RolloutGroupEvaluationManager(errorConditionEvaluators, successConditionEvaluators,
-                errorActionEvaluators, successActionEvaluators);
+        return new RolloutGroupEvaluationManager(
+                errorConditionEvaluators, successConditionEvaluators, errorActionEvaluators, successActionEvaluators);
     }
 
     @Bean
@@ -696,8 +696,8 @@ DistributionSetTagManagement distributionSetTagManagement(
             final DistributionSetTagRepository distributionSetTagRepository,
             final DistributionSetRepository distributionSetRepository,
             final VirtualPropertyReplacer virtualPropertyReplacer, final JpaProperties properties) {
-        return new JpaDistributionSetTagManagement(distributionSetTagRepository, distributionSetRepository,
-                virtualPropertyReplacer, properties.getDatabase());
+        return new JpaDistributionSetTagManagement(
+                distributionSetTagRepository, distributionSetRepository, virtualPropertyReplacer, properties.getDatabase());
     }
 
     /**
@@ -735,17 +735,15 @@ SoftwareModuleTypeManagement softwareModuleTypeManagement(
             final SoftwareModuleRepository softwareModuleRepository,
             final JpaProperties properties) {
         return new JpaSoftwareModuleTypeManagement(distributionSetTypeRepository, softwareModuleTypeRepository,
-                virtualPropertyReplacer, softwareModuleRepository,
-                properties.getDatabase());
+                virtualPropertyReplacer, softwareModuleRepository, properties.getDatabase());
     }
 
     @Bean
     @ConditionalOnMissingBean
     RolloutHandler rolloutHandler(final TenantAware tenantAware, final RolloutManagement rolloutManagement,
             final RolloutExecutor rolloutExecutor, final LockRegistry lockRegistry,
             final PlatformTransactionManager txManager, final ContextAware contextAware) {
-        return new JpaRolloutHandler(tenantAware, rolloutManagement, rolloutExecutor, lockRegistry, txManager,
-                contextAware);
+        return new JpaRolloutHandler(tenantAware, rolloutManagement, rolloutExecutor, lockRegistry, txManager, contextAware);
     }
 
     @Bean
@@ -777,8 +775,7 @@ RolloutManagement rolloutManagement(final TargetManagement targetManagement,
             final ContextAware contextAware) {
         return new JpaRolloutManagement(targetManagement, distributionSetManagement, eventPublisherHolder,
                 virtualPropertyReplacer, properties.getDatabase(), rolloutApprovalStrategy,
-                tenantConfigurationManagement, systemSecurityContext,
-                contextAware);
+                tenantConfigurationManagement, systemSecurityContext, contextAware);
     }
 
     /**
@@ -860,10 +857,12 @@ ControllerManagement controllerManagement(final ScheduledExecutorService executo
     @Bean
     @ConditionalOnMissingBean
     ArtifactManagement artifactManagement(
-            final EntityManager entityManager, final LocalArtifactRepository localArtifactRepository,
-            final SoftwareModuleRepository softwareModuleRepository, final Optional<ArtifactRepository> artifactRepository,
+            final EntityManager entityManager, final PlatformTransactionManager txManager,
+            final LocalArtifactRepository localArtifactRepository, final SoftwareModuleRepository softwareModuleRepository,
+            final Optional<ArtifactRepository> artifactRepository,
             final QuotaManagement quotaManagement, final TenantAware tenantAware) {
-        return new JpaArtifactManagement(entityManager, localArtifactRepository, softwareModuleRepository, artifactRepository.orElse(null),
+        return new JpaArtifactManagement(
+                entityManager, txManager, localArtifactRepository, softwareModuleRepository, artifactRepository.orElse(null),
                 quotaManagement, tenantAware);
     }
 

hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/executor/AfterTransactionCommitDefaultServiceExecutor.java

@@ -13,15 +13,15 @@
 import java.util.List;
 
 import lombok.extern.slf4j.Slf4j;
-import org.springframework.transaction.support.TransactionSynchronizationAdapter;
+import org.springframework.transaction.support.TransactionSynchronization;
 import org.springframework.transaction.support.TransactionSynchronizationManager;
 
 /**
- * A Service which calls register runnable. This runnables will executed after a
- * successful spring transaction commit.The class is thread safe.
+ * A Service which calls register runnable. This runnables will be executed after a successful spring transaction commit.
+ * The class is thread safe.
  */
 @Slf4j
-public class AfterTransactionCommitDefaultServiceExecutor extends TransactionSynchronizationAdapter implements AfterTransactionCommitExecutor {
+public class AfterTransactionCommitDefaultServiceExecutor implements TransactionSynchronization, AfterTransactionCommitExecutor {
 
     private static final ThreadLocal<List<Runnable>> THREAD_LOCAL_RUNNABLES = new ThreadLocal<>();
 
@@ -30,6 +30,14 @@ public class AfterTransactionCommitDefaultServiceExecutor extends TransactionSyn
     @SuppressWarnings({ "squid:S1217" })
     public void afterCommit() {
         final List<Runnable> afterCommitRunnables = THREAD_LOCAL_RUNNABLES.get();
+        if (afterCommitRunnables == null) {
+            log.trace("Transaction successfully committed, runnables is null");
+            return;
+        }
+
+        // removes the runnables that will process, so they would be able to start new transactions and
+        // inserting new after commit hooks
+        THREAD_LOCAL_RUNNABLES.remove();
         log.debug("Transaction successfully committed, executing {} runnables", afterCommitRunnables.size());
         for (final Runnable afterCommitRunnable : afterCommitRunnables) {
             log.debug("Executing runnable {}", afterCommitRunnable);
@@ -44,8 +52,7 @@ public void afterCommit() {
     @Override
     @SuppressWarnings({ "squid:S1217" })
     public void afterCompletion(final int status) {
-        final String transactionStatus = status == STATUS_COMMITTED ? "COMMITTED" : "ROLLEDBACK";
-        log.debug("Transaction completed after commit with status {}", transactionStatus);
+        log.debug("Transaction completed after commit with status {}", status == STATUS_COMMITTED ? "COMMITTED" : "ROLLEDBACK");
         THREAD_LOCAL_RUNNABLES.remove();
     }
 

hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/management/JpaArtifactManagement.java

@@ -36,26 +36,31 @@
 import org.eclipse.hawkbit.repository.exception.InvalidSHA1HashException;
 import org.eclipse.hawkbit.repository.exception.InvalidSHA256HashException;
 import org.eclipse.hawkbit.repository.jpa.EncryptionAwareDbArtifact;
+import org.eclipse.hawkbit.repository.jpa.Jpa;
 import org.eclipse.hawkbit.repository.jpa.JpaManagementHelper;
 import org.eclipse.hawkbit.repository.jpa.acm.AccessController;
 import org.eclipse.hawkbit.repository.jpa.configuration.Constants;
 import org.eclipse.hawkbit.repository.jpa.model.JpaArtifact;
 import org.eclipse.hawkbit.repository.jpa.model.JpaSoftwareModule;
+import org.eclipse.hawkbit.repository.jpa.model.helper.AfterTransactionCommitExecutorHolder;
 import org.eclipse.hawkbit.repository.jpa.repository.LocalArtifactRepository;
 import org.eclipse.hawkbit.repository.jpa.repository.SoftwareModuleRepository;
 import org.eclipse.hawkbit.repository.jpa.specifications.ArtifactSpecifications;
+import org.eclipse.hawkbit.repository.jpa.utils.DeploymentHelper;
 import org.eclipse.hawkbit.repository.jpa.utils.FileSizeAndStorageQuotaCheckingInputStream;
 import org.eclipse.hawkbit.repository.jpa.utils.QuotaHelper;
 import org.eclipse.hawkbit.repository.model.Artifact;
 import org.eclipse.hawkbit.repository.model.ArtifactUpload;
 import org.eclipse.hawkbit.repository.model.SoftwareModule;
 import org.eclipse.hawkbit.tenancy.TenantAware;
+import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.dao.ConcurrencyFailureException;
 import org.springframework.data.domain.Page;
 import org.springframework.data.domain.Pageable;
 import org.springframework.retry.annotation.Backoff;
 import org.springframework.retry.annotation.Retryable;
 import org.springframework.security.access.prepost.PreAuthorize;
+import org.springframework.transaction.PlatformTransactionManager;
 import org.springframework.transaction.annotation.Transactional;
 import org.springframework.transaction.support.TransactionSynchronization;
 import org.springframework.transaction.support.TransactionSynchronizationManager;
@@ -70,6 +75,7 @@
 public class JpaArtifactManagement implements ArtifactManagement {
 
     private final EntityManager entityManager;
+    private final PlatformTransactionManager txManager;
     private final LocalArtifactRepository localArtifactRepository;
     private final SoftwareModuleRepository softwareModuleRepository;
     @Nullable
@@ -79,10 +85,13 @@ public class JpaArtifactManagement implements ArtifactManagement {
 
     public JpaArtifactManagement(
             final EntityManager entityManager,
+            final PlatformTransactionManager txManager,
             final LocalArtifactRepository localArtifactRepository,
             final SoftwareModuleRepository softwareModuleRepository, @Nullable final ArtifactRepository artifactRepository,
-            final QuotaManagement quotaManagement, final TenantAware tenantAware) {
+            final QuotaManagement quotaManagement,
+            final TenantAware tenantAware) {
         this.entityManager = entityManager;
+        this.txManager = txManager;
         this.localArtifactRepository = localArtifactRepository;
         this.softwareModuleRepository = softwareModuleRepository;
         this.artifactRepository = artifactRepository;
@@ -137,18 +146,19 @@ public Artifact create(final ArtifactUpload artifactUpload) {
     @Retryable(retryFor = { ConcurrencyFailureException.class }, maxAttempts = Constants.TX_RT_MAX,
             backoff = @Backoff(delay = Constants.TX_RT_DELAY))
     public void delete(final long id) {
-        final JpaArtifact toDelete = (JpaArtifact) get(id)
-                .orElseThrow(() -> new EntityNotFoundException(Artifact.class, id));
+        final JpaArtifact toDelete = (JpaArtifact) get(id).orElseThrow(() -> new EntityNotFoundException(Artifact.class, id));
 
+        final JpaSoftwareModule softwareModule = toDelete.getSoftwareModule();
         // clearArtifactBinary checks (unconditionally) software module UPDATE access
         softwareModuleRepository.getAccessController().ifPresent(accessController ->
-                accessController.assertOperationAllowed(AccessController.Operation.UPDATE,
-                        (JpaSoftwareModule) toDelete.getSoftwareModule()));
-        ((JpaSoftwareModule) toDelete.getSoftwareModule()).removeArtifact(toDelete);
-        softwareModuleRepository.save((JpaSoftwareModule) toDelete.getSoftwareModule());
+                accessController.assertOperationAllowed(AccessController.Operation.UPDATE, softwareModule));
+        softwareModule.removeArtifact(toDelete);
+        softwareModuleRepository.save(softwareModule);
 
         localArtifactRepository.deleteById(id);
-        clearArtifactBinary(toDelete.getSha1Hash());
+
+        final String sha1Hash = toDelete.getSha1Hash();
+        AfterTransactionCommitExecutorHolder.getInstance().getAfterCommit().afterCommit(() -> clearArtifactBinary(sha1Hash));
     }
 
     @Override
@@ -211,39 +221,35 @@ public Optional<DbArtifact> loadArtifactBinary(final String sha1Hash, final long
     }
 
     /**
-     * Garbage collects artifact binaries if only referenced by given
-     * {@link SoftwareModule#getId()} or {@link SoftwareModule}'s that are
+     * Garbage collects artifact binaries if only referenced by given {@link SoftwareModule#getId()} or {@link SoftwareModule}'s that are
      * marked as deleted.
      * <p/>
      * Software module related UPDATE permission shall be checked by the callers!
+     * <p/>
+     * Note: Internal method. Shall be called ONLY if @PreAuthorize(SpPermission.SpringEvalExpressions.HAS_AUTH_DELETE_REPOSITORY)
+     * has already been checked
      *
      * @param sha1Hash no longer needed
      */
-    @PreAuthorize(SpPermission.SpringEvalExpressions.HAS_AUTH_DELETE_REPOSITORY)
     void clearArtifactBinary(final String sha1Hash) {
         assertArtifactRepositoryAvailable();
 
-        // countBySha1HashAndTenantAndSoftwareModuleDeletedIsFalse will skip ACM checks and
-        // will return total count as it should be
-        final long count = localArtifactRepository.countBySha1HashAndTenantAndSoftwareModuleDeletedIsFalse(
-                sha1Hash,
-                tenantAware.getCurrentTenant());
-        if (count <= 1) { // 1 artifact is the one being deleted!
-            // removes the real artifact ONLY AFTER the delete of artifact or software module
-            // in local history has passed successfully (caller has permission and no errors)
-            TransactionSynchronizationManager.registerSynchronization(new TransactionSynchronization() {
-
-                @Override
-                public void afterCommit() {
+        DeploymentHelper.runInNewTransaction(txManager, "clearArtifactBinary", status -> {
+            // countBySha1HashAndTenantAndSoftwareModuleDeletedIsFalse will skip ACM checks and will return total count as it should be
+            if (localArtifactRepository.countBySha1HashAndTenantAndSoftwareModuleDeletedIsFalse(sha1Hash, tenantAware.getCurrentTenant()) <= 0) { // 1 artifact is the one being deleted!
+                // removes the real artifact ONLY AFTER the delete of artifact or software module
+                // in local history has passed successfully (caller has permission and no errors)
+                AfterTransactionCommitExecutorHolder.getInstance().getAfterCommit().afterCommit(() -> {
                     try {
                         log.debug("deleting artifact from repository {}", sha1Hash);
                         artifactRepository.deleteBySha1(tenantAware.getCurrentTenant(), sha1Hash);
                     } catch (final ArtifactStoreException e) {
                         throw new ArtifactDeleteFailedException(e);
                     }
-                }
-            });
-        } // else there are still other artifacts that need the binary
+                });
+            } // else there are still other artifacts that need the binary
+            return null;
+        });
     }
 
     private AbstractDbArtifact storeArtifact(final ArtifactUpload artifactUpload, final boolean isSmEncrypted) {

hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/management/JpaSoftwareModuleManagement.java

@@ -48,6 +48,7 @@
 import org.eclipse.hawkbit.repository.jpa.model.JpaSoftwareModuleMetadata_;
 import org.eclipse.hawkbit.repository.jpa.model.JpaSoftwareModule_;
 import org.eclipse.hawkbit.repository.jpa.model.SwMetadataCompositeKey;
+import org.eclipse.hawkbit.repository.jpa.model.helper.AfterTransactionCommitExecutorHolder;
 import org.eclipse.hawkbit.repository.jpa.repository.DistributionSetRepository;
 import org.eclipse.hawkbit.repository.jpa.repository.SoftwareModuleMetadataRepository;
 import org.eclipse.hawkbit.repository.jpa.repository.SoftwareModuleRepository;
@@ -199,17 +200,15 @@ public void delete(final Collection<Long> ids) {
 
         final Set<Long> assignedModuleIds = new HashSet<>();
         swModulesToDelete.forEach(swModule -> {
-
-            // delete binary data of artifacts
-            deleteGridFsArtifacts(swModule);
-
             // execute this count operation without access limitations since we have to
             // ensure it's not assigned when deleting it.
             if (distributionSetRepository.countByModulesId(swModule.getId()) <= 0) {
                 softwareModuleRepository.deleteById(swModule.getId());
             } else {
                 assignedModuleIds.add(swModule.getId());
             }
+            // schedule delete binary data of artifacts
+            deleteGridFsArtifacts(swModule);
         });
 
         if (!assignedModuleIds.isEmpty()) {
@@ -507,10 +506,9 @@ private static Specification<JpaSoftwareModuleMetadata> metadataBySoftwareModule
     private void deleteGridFsArtifacts(final JpaSoftwareModule swModule) {
         softwareModuleRepository.getAccessController().ifPresent(accessController ->
                 accessController.assertOperationAllowed(AccessController.Operation.DELETE, swModule));
-        for (final Artifact localArtifact : swModule.getArtifacts()) {
-            ((JpaArtifactManagement) artifactManagement)
-                    .clearArtifactBinary(localArtifact.getSha1Hash());
-        }
+        final Set<String> sha1Hashes = swModule.getArtifacts().stream().map(Artifact::getSha1Hash).collect(Collectors.toSet());
+        AfterTransactionCommitExecutorHolder.getInstance().getAfterCommit().afterCommit(() ->
+                sha1Hashes.forEach(sha1Hash -> ((JpaArtifactManagement) artifactManagement).clearArtifactBinary(sha1Hash)));
     }
 
     private Specification<JpaSoftwareModule> buildSmSearchQuerySpec(final String searchText) {