when using rules to delete packages, remove unclean bugs

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,23 +283,26 @@ 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).
 		Select("package_version.*").
 		Table("package_version").
 		Join("INNER", "package", "package.id = package_version.package_id").
 		Where(opts.ToConds())
-
-	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
+	return searchVersionsBySession(sess, opts)
 }
 
 // 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)
-
-	if opts.Paginator != nil {
-		sess = db.SetSessionPagination(sess, opts)
-	}
-
-	pvs := make([]*PackageVersion, 0, 10)
-	count, err := sess.FindAndCount(&pvs)
-	return pvs, count, err
+	return searchVersionsBySession(sess, opts)
 }
 
 // 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,127 +32,136 @@ func CleanupTask(ctx context.Context, olderThan time.Duration) error {
 	return CleanupExpiredData(ctx, olderThan)
 }
 
-func ExecuteCleanupRules(outerCtx context.Context) error {
-	ctx, committer, err := db.TxContext(outerCtx)
+func executeCleanupOneRulePackage(ctx context.Context, pcr *packages_model.PackageCleanupRule, p *packages_model.Package) (versionDeleted bool, err error) {
+	olderThan := time.Now().AddDate(0, 0, -pcr.RemoveDays)
+	pvs, _, err := packages_model.SearchVersions(ctx, &packages_model.PackageSearchOptions{
+		PackageID:  p.ID,
+		IsInternal: optional.Some(false),
+		Sort:       packages_model.SortCreatedDesc,
+	})
 	if err != nil {
-		return err
+		return false, fmt.Errorf("CleanupRule [%d]: SearchVersions failed: %w", pcr.ID, 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 pcr.KeepCount > 0 {
+		if pcr.KeepCount < len(pvs) {
+			pvs = pvs[pcr.KeepCount:]
+		} else {
+			pvs = nil
 		}
-
-		if err := pcr.CompiledPattern(); err != nil {
-			return fmt.Errorf("CleanupRule [%d]: CompilePattern failed: %w", pcr.ID, err)
+	}
+	for _, pv := range pvs {
+		if pcr.Type == packages_model.TypeContainer {
+			if skip, err := container_service.ShouldBeSkipped(ctx, pcr, p, pv); err != nil {
+				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
+			}
 		}
-
-		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)
+		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) %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 {
+			log.Error("CleanupRule [%d]: DeletePackageVersionAndReferences failed: %v", pcr.ID, err)
+			continue
+		}
+		versionDeleted = true
+	}
+	return versionDeleted, nil
+}
 
-		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)
-			}
-			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)
-					} else if skip {
-						log.Debug("Rule[%d]: keep '%s/%s' (container)", pcr.ID, p.Name, pv.Version)
-						continue
-					}
-				}
+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)
+	}
 
-				toMatch := pv.LowerVersion
-				if pcr.MatchFullName {
-					toMatch = p.LowerName + "/" + pv.LowerVersion
-				}
+	packages, err := packages_model.GetPackagesByType(ctx, pcr.OwnerID, pcr.Type)
+	if err != nil {
+		return fmt.Errorf("CleanupRule [%d]: GetPackagesByType failed: %w", pcr.ID, err)
+	}
 
-				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)
-					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
+	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)
+				if err != nil {
+					return fmt.Errorf("GetUserByID failed: %w", err)
 				}
-
-				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)
+				if err := cargo_service.UpdatePackageIndexIfExists(ctx, owner, owner, p.ID); err != nil {
+					return fmt.Errorf("CleanupRule [%d]: cargo.UpdatePackageIndexIfExists failed: %w", pcr.ID, err)
 				}
+			}
+		}
+	}
 
-				versionDeleted = true
-				anyVersionDeleted = true
+	if anyVersionDeleted {
+		switch pcr.Type {
+		case packages_model.TypeDebian:
+			if err := debian_service.BuildAllRepositoryFiles(ctx, pcr.OwnerID); err != nil {
+				return fmt.Errorf("CleanupRule [%d]: debian.BuildAllRepositoryFiles failed: %w", pcr.ID, err)
+			}
+		case packages_model.TypeAlpine:
+			if err := alpine_service.BuildAllRepositoryFiles(ctx, pcr.OwnerID); err != nil {
+				return fmt.Errorf("CleanupRule [%d]: alpine.BuildAllRepositoryFiles failed: %w", pcr.ID, err)
 			}
+		case packages_model.TypeRpm:
+			if err := rpm_service.BuildAllRepositoryFiles(ctx, pcr.OwnerID); err != nil {
+				return fmt.Errorf("CleanupRule [%d]: rpm.BuildAllRepositoryFiles failed: %w", pcr.ID, err)
+			}
+		case packages_model.TypeArch:
+			release, err := arch_service.AquireRegistryLock(ctx, pcr.OwnerID)
+			if err != nil {
+				return err
+			}
+			defer release()
 
-			if versionDeleted {
-				if pcr.Type == packages_model.TypeCargo {
-					owner, err := user_model.GetUserByID(ctx, pcr.OwnerID)
-					if err != nil {
-						return fmt.Errorf("GetUserByID failed: %w", err)
-					}
-					if err := cargo_service.UpdatePackageIndexIfExists(ctx, owner, owner, p.ID); err != nil {
-						return fmt.Errorf("CleanupRule [%d]: cargo.UpdatePackageIndexIfExists failed: %w", pcr.ID, err)
-					}
-				}
+			if err := arch_service.BuildAllRepositoryFiles(ctx, pcr.OwnerID); err != nil {
+				return fmt.Errorf("CleanupRule [%d]: arch.BuildAllRepositoryFiles failed: %w", pcr.ID, err)
 			}
 		}
+	}
+	return nil
+}
 
-		if anyVersionDeleted {
-			switch pcr.Type {
-			case packages_model.TypeDebian:
-				if err := debian_service.BuildAllRepositoryFiles(ctx, pcr.OwnerID); err != nil {
-					return fmt.Errorf("CleanupRule [%d]: debian.BuildAllRepositoryFiles failed: %w", pcr.ID, err)
-				}
-			case packages_model.TypeAlpine:
-				if err := alpine_service.BuildAllRepositoryFiles(ctx, pcr.OwnerID); err != nil {
-					return fmt.Errorf("CleanupRule [%d]: alpine.BuildAllRepositoryFiles failed: %w", pcr.ID, err)
-				}
-			case packages_model.TypeRpm:
-				if err := rpm_service.BuildAllRepositoryFiles(ctx, pcr.OwnerID); err != nil {
-					return fmt.Errorf("CleanupRule [%d]: rpm.BuildAllRepositoryFiles failed: %w", pcr.ID, err)
-				}
-			case packages_model.TypeArch:
-				release, err := arch_service.AquireRegistryLock(ctx, pcr.OwnerID)
-				if err != nil {
-					return err
-				}
-				defer release()
+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:
+		}
 
-				if err := arch_service.BuildAllRepositoryFiles(ctx, pcr.OwnerID); err != nil {
-					return fmt.Errorf("CleanupRule [%d]: arch.BuildAllRepositoryFiles failed: %w", pcr.ID, err)
-				}
-			}
+		err := executeCleanupOneRule(ctx, pcr)
+		if err != nil {
+			log.Error("CleanupRule [%d]: executeCleanupOneRule failed: %v", pcr.ID, err)
 		}
 		return nil
 	})
-	if err != nil {
-		return err
-	}
-
-	return committer.Commit()
 }
 
 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{
-					{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},
-				},
+				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},
+					}
+					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)
 					}
 				}