From d1fdbf46bd367f4f1acd9327286df2739183965c Mon Sep 17 00:00:00 2001 From: Giteabot Date: Wed, 18 Jun 2025 14:18:55 +0800 Subject: [PATCH] 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 --- models/packages/nuget/search.go | 2 +- models/packages/package_version.go | 36 ++-- routers/web/shared/packages/packages.go | 10 +- services/packages/cleanup/cleanup.go | 227 ++++++++++++------------ tests/integration/api_packages_test.go | 18 +- 5 files changed, 153 insertions(+), 140 deletions(-) diff --git a/models/packages/nuget/search.go b/models/packages/nuget/search.go index 7a505ff08f..a4b23f31d5 100644 --- a/models/packages/nuget/search.go +++ b/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) } diff --git a/models/packages/package_version.go b/models/packages/package_version.go index bb7fd895f8..5672e0efbf 100644 --- a/models/packages/package_version.go +++ b/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()) - - 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 diff --git a/routers/web/shared/packages/packages.go b/routers/web/shared/packages/packages.go index 3d1795b42c..a18dedf89c 100644 --- a/routers/web/shared/packages/packages.go +++ b/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 } diff --git a/services/packages/cleanup/cleanup.go b/services/packages/cleanup/cleanup.go index b7ba2b6ac4..959babe7cd 100644 --- a/services/packages/cleanup/cleanup.go +++ b/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() + if pcr.KeepCount > 0 { + if pcr.KeepCount < len(pvs) { + pvs = pvs[pcr.KeepCount:] + } else { + pvs = nil + } + } + 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 + } + } + 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 +} - err = packages_model.IterateEnabledCleanupRules(ctx, func(ctx context.Context, pcr *packages_model.PackageCleanupRule) error { +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) + 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 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 err := arch_service.BuildAllRepositoryFiles(ctx, pcr.OwnerID); err != nil { + return fmt.Errorf("CleanupRule [%d]: arch.BuildAllRepositoryFiles failed: %w", pcr.ID, err) + } + } + } + return nil +} + +func ExecuteCleanupRules(ctx context.Context) error { + return packages_model.IterateEnabledCleanupRules(ctx, func(ctx context.Context, pcr *packages_model.PackageCleanupRule) error { select { - case <-outerCtx.Done(): + case <-ctx.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) + err := executeCleanupOneRule(ctx, pcr) 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) - } - 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 - } - } - - 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) - 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) - } - - versionDeleted = true - anyVersionDeleted = true - } - - 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 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 err := arch_service.BuildAllRepositoryFiles(ctx, pcr.OwnerID); err != nil { - return fmt.Errorf("CleanupRule [%d]: arch.BuildAllRepositoryFiles failed: %w", pcr.ID, err) - } - } + 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 { diff --git a/tests/integration/api_packages_test.go b/tests/integration/api_packages_test.go index 786addbd76..106b0befea 100644 --- a/tests/integration/api_packages_test.go +++ b/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) } }