when using rules to delete packages, remove unclean bugs (#34632) (#34761)

Backport #34632 by @anthony-zh

By default, the code extracts 200 package versions. If too many packages
are generated every day or if rule cleaning is enabled later, which
means there are more than 200 versions corresponding to the library
package, it may not be cleaned up completely, resulting in residue

Fix #31961

Co-authored-by: anthony-zh <118415914+anthony-zh@users.noreply.github.com>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

models/packages/nuget/search.go

@@ -33,7 +33,7 @@ func SearchVersions(ctx context.Context, opts *packages_model.PackageSearchOptio
 		Where(cond).
 		OrderBy("package.name ASC")
 	if opts.Paginator != nil {
-		skip, take := opts.GetSkipTake()
+		skip, take := opts.Paginator.GetSkipTake()
 		inner = inner.Limit(take, skip)
 	}
 

models/packages/package_version.go

@@ -14,6 +14,7 @@ import (
 	"code.gitea.io/gitea/modules/util"
 
 	"xorm.io/builder"
+	"xorm.io/xorm"
 )
 
 // ErrDuplicatePackageVersion indicates a duplicated package version error
@@ -187,7 +188,7 @@ type PackageSearchOptions struct {
 	HasFileWithName string                // only results are found which are associated with a file with the specific name
 	HasFiles        optional.Option[bool] // only results are found which have associated files
 	Sort            VersionSort
-	db.Paginator
+	Paginator       db.Paginator
 }
 
 func (opts *PackageSearchOptions) ToConds() builder.Cond {
@@ -282,6 +283,18 @@ func (opts *PackageSearchOptions) configureOrderBy(e db.Engine) {
 	e.Desc("package_version.id") // Sort by id for stable order with duplicates in the other field
 }
 
+func searchVersionsBySession(sess *xorm.Session, opts *PackageSearchOptions) ([]*PackageVersion, int64, error) {
+	opts.configureOrderBy(sess)
+	pvs := make([]*PackageVersion, 0, 10)
+	if opts.Paginator != nil {
+		sess = db.SetSessionPagination(sess, opts.Paginator)
+		count, err := sess.FindAndCount(&pvs)
+		return pvs, count, err
+	}
+	err := sess.Find(&pvs)
+	return pvs, int64(len(pvs)), err
+}
+
 // SearchVersions gets all versions of packages matching the search options
 func SearchVersions(ctx context.Context, opts *PackageSearchOptions) ([]*PackageVersion, int64, error) {
 	sess := db.GetEngine(ctx).
@@ -289,16 +302,7 @@ func SearchVersions(ctx context.Context, opts *PackageSearchOptions) ([]*Package
 		Table("package_version").
 		Join("INNER", "package", "package.id = package_version.package_id").
 		Where(opts.ToConds())
-
+	return searchVersionsBySession(sess, opts)
-	opts.configureOrderBy(sess)
-
-	if opts.Paginator != nil {
-		sess = db.SetSessionPagination(sess, opts)
-	}
-
-	pvs := make([]*PackageVersion, 0, 10)
-	count, err := sess.FindAndCount(&pvs)
-	return pvs, count, err
 }
 
 // SearchLatestVersions gets the latest version of every package matching the search options
@@ -316,15 +320,7 @@ func SearchLatestVersions(ctx context.Context, opts *PackageSearchOptions) ([]*P
 		Join("INNER", "package", "package.id = package_version.package_id").
 		Where(builder.In("package_version.id", in))
 
-	opts.configureOrderBy(sess)
+	return searchVersionsBySession(sess, opts)
-
-	if opts.Paginator != nil {
-		sess = db.SetSessionPagination(sess, opts)
-	}
-
-	pvs := make([]*PackageVersion, 0, 10)
-	count, err := sess.FindAndCount(&pvs)
-	return pvs, count, err
 }
 
 // ExistVersion checks if a version matching the search options exist

routers/web/shared/packages/packages.go

@@ -8,7 +8,6 @@ import (
 	"net/http"
 	"time"
 
-	"code.gitea.io/gitea/models/db"
 	packages_model "code.gitea.io/gitea/models/packages"
 	user_model "code.gitea.io/gitea/models/user"
 	"code.gitea.io/gitea/modules/log"
@@ -159,12 +158,18 @@ func SetRulePreviewContext(ctx *context.Context, owner *user_model.User) {
 			PackageID:  p.ID,
 			IsInternal: optional.Some(false),
 			Sort:       packages_model.SortCreatedDesc,
-			Paginator:  db.NewAbsoluteListOptions(pcr.KeepCount, 200),
 		})
 		if err != nil {
 			ctx.ServerError("SearchVersions", err)
 			return
 		}
+		if pcr.KeepCount > 0 {
+			if pcr.KeepCount < len(pvs) {
+				pvs = pvs[pcr.KeepCount:]
+			} else {
+				pvs = nil
+			}
+		}
 		for _, pv := range pvs {
 			if skip, err := container_service.ShouldBeSkipped(ctx, pcr, p, pv); err != nil {
 				ctx.ServerError("ShouldBeSkipped", err)
@@ -177,7 +182,6 @@ func SetRulePreviewContext(ctx *context.Context, owner *user_model.User) {
 			if pcr.MatchFullName {
 				toMatch = p.LowerName + "/" + pv.LowerVersion
 			}
-
 			if pcr.KeepPatternMatcher != nil && pcr.KeepPatternMatcher.MatchString(toMatch) {
 				continue
 			}

services/packages/cleanup/cleanup.go

@@ -32,81 +32,80 @@ func CleanupTask(ctx context.Context, olderThan time.Duration) error {
 	return CleanupExpiredData(ctx, olderThan)
 }
 
-func ExecuteCleanupRules(outerCtx context.Context) error {
+func executeCleanupOneRulePackage(ctx context.Context, pcr *packages_model.PackageCleanupRule, p *packages_model.Package) (versionDeleted bool, err error) {
-	ctx, committer, err := db.TxContext(outerCtx)
-	if err != nil {
-		return err
-	}
-	defer committer.Close()
-
-	err = packages_model.IterateEnabledCleanupRules(ctx, func(ctx context.Context, pcr *packages_model.PackageCleanupRule) error {
-		select {
-		case <-outerCtx.Done():
-			return db.ErrCancelledf("While processing package cleanup rules")
-		default:
-		}
-
-		if err := pcr.CompiledPattern(); err != nil {
-			return fmt.Errorf("CleanupRule [%d]: CompilePattern failed: %w", pcr.ID, err)
-		}
-
 	olderThan := time.Now().AddDate(0, 0, -pcr.RemoveDays)
-
-		packages, err := packages_model.GetPackagesByType(ctx, pcr.OwnerID, pcr.Type)
-		if err != nil {
-			return fmt.Errorf("CleanupRule [%d]: GetPackagesByType failed: %w", pcr.ID, err)
-		}
-
-		anyVersionDeleted := false
-		for _, p := range packages {
 	pvs, _, err := packages_model.SearchVersions(ctx, &packages_model.PackageSearchOptions{
 		PackageID:  p.ID,
 		IsInternal: optional.Some(false),
 		Sort:       packages_model.SortCreatedDesc,
-				Paginator:  db.NewAbsoluteListOptions(pcr.KeepCount, 200),
 	})
 	if err != nil {
-				return fmt.Errorf("CleanupRule [%d]: SearchVersions failed: %w", pcr.ID, err)
+		return false, fmt.Errorf("CleanupRule [%d]: SearchVersions failed: %w", pcr.ID, err)
+	}
+	if pcr.KeepCount > 0 {
+		if pcr.KeepCount < len(pvs) {
+			pvs = pvs[pcr.KeepCount:]
+		} else {
+			pvs = nil
+		}
 	}
-			versionDeleted := false
 	for _, pv := range pvs {
 		if pcr.Type == packages_model.TypeContainer {
 			if skip, err := container_service.ShouldBeSkipped(ctx, pcr, p, pv); err != nil {
-						return fmt.Errorf("CleanupRule [%d]: container.ShouldBeSkipped failed: %w", pcr.ID, err)
+				return false, fmt.Errorf("CleanupRule [%d]: container.ShouldBeSkipped failed: %w", pcr.ID, err)
 			} else if skip {
 				log.Debug("Rule[%d]: keep '%s/%s' (container)", pcr.ID, p.Name, pv.Version)
 				continue
 			}
 		}
-
 		toMatch := pv.LowerVersion
 		if pcr.MatchFullName {
 			toMatch = p.LowerName + "/" + pv.LowerVersion
 		}
-
 		if pcr.KeepPatternMatcher != nil && pcr.KeepPatternMatcher.MatchString(toMatch) {
 			log.Debug("Rule[%d]: keep '%s/%s' (keep pattern)", pcr.ID, p.Name, pv.Version)
 			continue
 		}
 		if pv.CreatedUnix.AsLocalTime().After(olderThan) {
-					log.Debug("Rule[%d]: keep '%s/%s' (remove days)", pcr.ID, p.Name, pv.Version)
+			log.Debug("Rule[%d]: keep '%s/%s' (remove days) %v", pcr.ID, p.Name, pv.Version, pv.CreatedUnix.FormatDate())
 			continue
 		}
 		if pcr.RemovePatternMatcher != nil && !pcr.RemovePatternMatcher.MatchString(toMatch) {
 			log.Debug("Rule[%d]: keep '%s/%s' (remove pattern)", pcr.ID, p.Name, pv.Version)
 			continue
 		}
-
 		log.Debug("Rule[%d]: remove '%s/%s'", pcr.ID, p.Name, pv.Version)
-
 		if err := packages_service.DeletePackageVersionAndReferences(ctx, pv); err != nil {
-					return fmt.Errorf("CleanupRule [%d]: DeletePackageVersionAndReferences failed: %w", pcr.ID, err)
+			log.Error("CleanupRule [%d]: DeletePackageVersionAndReferences failed: %v", pcr.ID, err)
+			continue
 		}
-
 		versionDeleted = true
-				anyVersionDeleted = true
+	}
+	return versionDeleted, nil
+}
+
+func executeCleanupOneRule(ctx context.Context, pcr *packages_model.PackageCleanupRule) error {
+	if err := pcr.CompiledPattern(); err != nil {
+		return fmt.Errorf("CleanupRule [%d]: CompilePattern failed: %w", pcr.ID, err)
+	}
+
+	packages, err := packages_model.GetPackagesByType(ctx, pcr.OwnerID, pcr.Type)
+	if err != nil {
+		return fmt.Errorf("CleanupRule [%d]: GetPackagesByType failed: %w", pcr.ID, err)
 	}
 
+	anyVersionDeleted := false
+	for _, p := range packages {
+		versionDeleted := false
+		err = db.WithTx(ctx, func(ctx context.Context) (err error) {
+			versionDeleted, err = executeCleanupOneRulePackage(ctx, pcr, p)
+			return err
+		})
+		if err != nil {
+			log.Error("CleanupRule [%d]: executeCleanupOneRulePackage(%d) failed: %v", pcr.ID, p.ID, err)
+			continue
+		}
+		anyVersionDeleted = anyVersionDeleted || versionDeleted
 		if versionDeleted {
 			if pcr.Type == packages_model.TypeCargo {
 				owner, err := user_model.GetUserByID(ctx, pcr.OwnerID)
@@ -147,12 +146,22 @@ func ExecuteCleanupRules(outerCtx context.Context) error {
 		}
 	}
 	return nil
-	})
-	if err != nil {
-		return err
 }
 
-	return committer.Commit()
+func ExecuteCleanupRules(ctx context.Context) error {
+	return packages_model.IterateEnabledCleanupRules(ctx, func(ctx context.Context, pcr *packages_model.PackageCleanupRule) error {
+		select {
+		case <-ctx.Done():
+			return db.ErrCancelledf("While processing package cleanup rules")
+		default:
+		}
+
+		err := executeCleanupOneRule(ctx, pcr)
+		if err != nil {
+			log.Error("CleanupRule [%d]: executeCleanupOneRule failed: %v", pcr.ID, err)
+		}
+		return nil
+	})
 }
 
 func CleanupExpiredData(outerCtx context.Context, olderThan time.Duration) error {

tests/integration/api_packages_test.go

@@ -636,12 +636,16 @@ func TestPackageCleanup(t *testing.T) {
 			},
 			{
 				Name: "Mixed",
-				Versions: []version{
+				Versions: func(limit, removeDays int) []version {
+					aa := []version{
 						{Version: "keep", ShouldExist: true, Created: time.Now().Add(time.Duration(10000)).Unix()},
 						{Version: "dummy", ShouldExist: true, Created: 1},
-					{Version: "test-3", ShouldExist: true},
+					}
-					{Version: "test-4", ShouldExist: false, Created: 1},
+					for i := range limit {
-				},
+						aa = append(aa, version{Version: fmt.Sprintf("test-%v", i+3), ShouldExist: util.Iif(i < removeDays, true, false), Created: time.Now().AddDate(0, 0, -i).Unix()})
+					}
+					return aa
+				}(220, 7),
 				Rule: &packages_model.PackageCleanupRule{
 					Enabled:       true,
 					KeepCount:     1,
@@ -686,7 +690,7 @@ func TestPackageCleanup(t *testing.T) {
 						err = packages_service.DeletePackageVersionAndReferences(db.DefaultContext, pv)
 						assert.NoError(t, err)
 					} else {
-						assert.ErrorIs(t, err, packages_model.ErrPackageNotExist)
+						assert.ErrorIs(t, err, packages_model.ErrPackageNotExist, v.Version)
 					}
 				}