Expire artifacts before deleting them physically (#29241)

#27172 (comment)

When cleanup artifacts, it removes storage first. If storage is not
exist (maybe delete manually), it gets error and continue loop. It makes
a dead loop if there are a lot pending but non-existing artifacts.

Now it updates db record at first to avoid keep a lot of pending status
artifacts.

Author: fuxiaohei

Diff for: services/actions/cleanup.go

@@ -35,14 +35,14 @@ func cleanExpiredArtifacts(taskCtx context.Context) error {
 	}
 	log.Info("Found %d expired artifacts", len(artifacts))
 	for _, artifact := range artifacts {
-		if err := storage.ActionsArtifacts.Delete(artifact.StoragePath); err != nil {
-			log.Error("Cannot delete artifact %d: %v", artifact.ID, err)
-			continue
-		}
 		if err := actions.SetArtifactExpired(taskCtx, artifact.ID); err != nil {
 			log.Error("Cannot set artifact %d expired: %v", artifact.ID, err)
 			continue
 		}
+		if err := storage.ActionsArtifacts.Delete(artifact.StoragePath); err != nil {
+			log.Error("Cannot delete artifact %d: %v", artifact.ID, err)
+			continue
+		}
 		log.Info("Artifact %d set expired", artifact.ID)
 	}
 	return nil
@@ -59,14 +59,14 @@ func cleanNeedDeleteArtifacts(taskCtx context.Context) error {
 		}
 		log.Info("Found %d artifacts pending deletion", len(artifacts))
 		for _, artifact := range artifacts {
-			if err := storage.ActionsArtifacts.Delete(artifact.StoragePath); err != nil {
-				log.Error("Cannot delete artifact %d: %v", artifact.ID, err)
-				continue
-			}
 			if err := actions.SetArtifactDeleted(taskCtx, artifact.ID); err != nil {
 				log.Error("Cannot set artifact %d deleted: %v", artifact.ID, err)
 				continue
 			}
+			if err := storage.ActionsArtifacts.Delete(artifact.StoragePath); err != nil {
+				log.Error("Cannot delete artifact %d: %v", artifact.ID, err)
+				continue
+			}
 			log.Info("Artifact %d set deleted", artifact.ID)
 		}
 		if len(artifacts) < deleteArtifactBatchSize {