Skip to content

Commit 0e6c122

Browse files
anthony-zhyeyuanjiewxiaoguang
authored
when using rules to delete packages, remove unclean bugs (#34632)
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: yeyuanjie <yecao100@126.com> Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
1 parent b388138 commit 0e6c122

File tree

5 files changed

+145
-132
lines changed

5 files changed

+145
-132
lines changed

‎models/packages/nuget/search.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ func SearchVersions(ctx context.Context, opts *packages_model.PackageSearchOptio
3333
Where(cond).
3434
OrderBy("package.name ASC")
3535
if opts.Paginator != nil {
36-
skip, take := opts.GetSkipTake()
36+
skip, take := opts.Paginator.GetSkipTake()
3737
inner = inner.Limit(take, skip)
3838
}
3939

‎models/packages/package_version.go

Lines changed: 16 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import (
1414
"code.gitea.io/gitea/modules/util"
1515

1616
"xorm.io/builder"
17+
"xorm.io/xorm"
1718
)
1819

1920
// ErrDuplicatePackageVersion indicates a duplicated package version error
@@ -187,7 +188,7 @@ type PackageSearchOptions struct {
187188
HasFileWithName string // only results are found which are associated with a file with the specific name
188189
HasFiles optional.Option[bool] // only results are found which have associated files
189190
Sort VersionSort
190-
db.Paginator
191+
Paginator db.Paginator
191192
}
192193

193194
func (opts *PackageSearchOptions) ToConds() builder.Cond {
@@ -282,23 +283,26 @@ func (opts *PackageSearchOptions) configureOrderBy(e db.Engine) {
282283
e.Desc("package_version.id") // Sort by id for stable order with duplicates in the other field
283284
}
284285

286+
func searchVersionsBySession(sess *xorm.Session, opts *PackageSearchOptions) ([]*PackageVersion, int64, error) {
287+
opts.configureOrderBy(sess)
288+
pvs := make([]*PackageVersion, 0, 10)
289+
if opts.Paginator != nil {
290+
sess = db.SetSessionPagination(sess, opts.Paginator)
291+
count, err := sess.FindAndCount(&pvs)
292+
return pvs, count, err
293+
}
294+
err := sess.Find(&pvs)
295+
return pvs, int64(len(pvs)), err
296+
}
297+
285298
// SearchVersions gets all versions of packages matching the search options
286299
func SearchVersions(ctx context.Context, opts *PackageSearchOptions) ([]*PackageVersion, int64, error) {
287300
sess := db.GetEngine(ctx).
288301
Select("package_version.*").
289302
Table("package_version").
290303
Join("INNER", "package", "package.id = package_version.package_id").
291304
Where(opts.ToConds())
292-
293-
opts.configureOrderBy(sess)
294-
295-
if opts.Paginator != nil {
296-
sess = db.SetSessionPagination(sess, opts)
297-
}
298-
299-
pvs := make([]*PackageVersion, 0, 10)
300-
count, err := sess.FindAndCount(&pvs)
301-
return pvs, count, err
305+
return searchVersionsBySession(sess, opts)
302306
}
303307

304308
// SearchLatestVersions gets the latest version of every package matching the search options
@@ -316,15 +320,7 @@ func SearchLatestVersions(ctx context.Context, opts *PackageSearchOptions) ([]*P
316320
Join("INNER", "package", "package.id = package_version.package_id").
317321
Where(builder.In("package_version.id", in))
318322

319-
opts.configureOrderBy(sess)
320-
321-
if opts.Paginator != nil {
322-
sess = db.SetSessionPagination(sess, opts)
323-
}
324-
325-
pvs := make([]*PackageVersion, 0, 10)
326-
count, err := sess.FindAndCount(&pvs)
327-
return pvs, count, err
323+
return searchVersionsBySession(sess, opts)
328324
}
329325

330326
// ExistVersion checks if a version matching the search options exist

‎routers/web/shared/packages/packages.go

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@ import (
88
"net/http"
99
"time"
1010

11-
"code.gitea.io/gitea/models/db"
1211
packages_model "code.gitea.io/gitea/models/packages"
1312
user_model "code.gitea.io/gitea/models/user"
1413
"code.gitea.io/gitea/modules/log"
@@ -159,12 +158,18 @@ func SetRulePreviewContext(ctx *context.Context, owner *user_model.User) {
159158
PackageID: p.ID,
160159
IsInternal: optional.Some(false),
161160
Sort: packages_model.SortCreatedDesc,
162-
Paginator: db.NewAbsoluteListOptions(pcr.KeepCount, 200),
163161
})
164162
if err != nil {
165163
ctx.ServerError("SearchVersions", err)
166164
return
167165
}
166+
if pcr.KeepCount > 0 {
167+
if pcr.KeepCount < len(pvs) {
168+
pvs = pvs[pcr.KeepCount:]
169+
} else {
170+
pvs = nil
171+
}
172+
}
168173
for _, pv := range pvs {
169174
if skip, err := container_service.ShouldBeSkipped(ctx, pcr, p, pv); err != nil {
170175
ctx.ServerError("ShouldBeSkipped", err)
@@ -177,7 +182,6 @@ func SetRulePreviewContext(ctx *context.Context, owner *user_model.User) {
177182
if pcr.MatchFullName {
178183
toMatch = p.LowerName + "/" + pv.LowerVersion
179184
}
180-
181185
if pcr.KeepPatternMatcher != nil && pcr.KeepPatternMatcher.MatchString(toMatch) {
182186
continue
183187
}

‎services/packages/cleanup/cleanup.go

Lines changed: 110 additions & 101 deletions
Original file line numberDiff line numberDiff line change
@@ -32,127 +32,136 @@ func CleanupTask(ctx context.Context, olderThan time.Duration) error {
3232
return CleanupExpiredData(ctx, olderThan)
3333
}
3434

35-
func ExecuteCleanupRules(outerCtx context.Context) error {
36-
ctx, committer, err := db.TxContext(outerCtx)
35+
func executeCleanupOneRulePackage(ctx context.Context, pcr *packages_model.PackageCleanupRule, p *packages_model.Package) (versionDeleted bool, err error) {
36+
olderThan := time.Now().AddDate(0, 0, -pcr.RemoveDays)
37+
pvs, _, err := packages_model.SearchVersions(ctx, &packages_model.PackageSearchOptions{
38+
PackageID: p.ID,
39+
IsInternal: optional.Some(false),
40+
Sort: packages_model.SortCreatedDesc,
41+
})
3742
if err != nil {
38-
return err
43+
return false, fmt.Errorf("CleanupRule [%d]: SearchVersions failed: %w", pcr.ID, err)
3944
}
40-
defer committer.Close()
41-
42-
err = packages_model.IterateEnabledCleanupRules(ctx, func(ctx context.Context, pcr *packages_model.PackageCleanupRule) error {
43-
select {
44-
case <-outerCtx.Done():
45-
return db.ErrCancelledf("While processing package cleanup rules")
46-
default:
45+
if pcr.KeepCount > 0 {
46+
if pcr.KeepCount < len(pvs) {
47+
pvs = pvs[pcr.KeepCount:]
48+
} else {
49+
pvs = nil
4750
}
48-
49-
if err := pcr.CompiledPattern(); err != nil {
50-
return fmt.Errorf("CleanupRule [%d]: CompilePattern failed: %w", pcr.ID, err)
51+
}
52+
for _, pv := range pvs {
53+
if pcr.Type == packages_model.TypeContainer {
54+
if skip, err := container_service.ShouldBeSkipped(ctx, pcr, p, pv); err != nil {
55+
return false, fmt.Errorf("CleanupRule [%d]: container.ShouldBeSkipped failed: %w", pcr.ID, err)
56+
} else if skip {
57+
log.Debug("Rule[%d]: keep '%s/%s' (container)", pcr.ID, p.Name, pv.Version)
58+
continue
59+
}
5160
}
52-
53-
olderThan := time.Now().AddDate(0, 0, -pcr.RemoveDays)
54-
55-
packages, err := packages_model.GetPackagesByType(ctx, pcr.OwnerID, pcr.Type)
56-
if err != nil {
57-
return fmt.Errorf("CleanupRule [%d]: GetPackagesByType failed: %w", pcr.ID, err)
61+
toMatch := pv.LowerVersion
62+
if pcr.MatchFullName {
63+
toMatch = p.LowerName + "/" + pv.LowerVersion
64+
}
65+
if pcr.KeepPatternMatcher != nil && pcr.KeepPatternMatcher.MatchString(toMatch) {
66+
log.Debug("Rule[%d]: keep '%s/%s' (keep pattern)", pcr.ID, p.Name, pv.Version)
67+
continue
68+
}
69+
if pv.CreatedUnix.AsLocalTime().After(olderThan) {
70+
log.Debug("Rule[%d]: keep '%s/%s' (remove days) %v", pcr.ID, p.Name, pv.Version, pv.CreatedUnix.FormatDate())
71+
continue
5872
}
73+
if pcr.RemovePatternMatcher != nil && !pcr.RemovePatternMatcher.MatchString(toMatch) {
74+
log.Debug("Rule[%d]: keep '%s/%s' (remove pattern)", pcr.ID, p.Name, pv.Version)
75+
continue
76+
}
77+
log.Debug("Rule[%d]: remove '%s/%s'", pcr.ID, p.Name, pv.Version)
78+
if err := packages_service.DeletePackageVersionAndReferences(ctx, pv); err != nil {
79+
log.Error("CleanupRule [%d]: DeletePackageVersionAndReferences failed: %v", pcr.ID, err)
80+
continue
81+
}
82+
versionDeleted = true
83+
}
84+
return versionDeleted, nil
85+
}
5986

60-
anyVersionDeleted := false
61-
for _, p := range packages {
62-
pvs, _, err := packages_model.SearchVersions(ctx, &packages_model.PackageSearchOptions{
63-
PackageID: p.ID,
64-
IsInternal: optional.Some(false),
65-
Sort: packages_model.SortCreatedDesc,
66-
Paginator: db.NewAbsoluteListOptions(pcr.KeepCount, 200),
67-
})
68-
if err != nil {
69-
return fmt.Errorf("CleanupRule [%d]: SearchVersions failed: %w", pcr.ID, err)
70-
}
71-
versionDeleted := false
72-
for _, pv := range pvs {
73-
if pcr.Type == packages_model.TypeContainer {
74-
if skip, err := container_service.ShouldBeSkipped(ctx, pcr, p, pv); err != nil {
75-
return fmt.Errorf("CleanupRule [%d]: container.ShouldBeSkipped failed: %w", pcr.ID, err)
76-
} else if skip {
77-
log.Debug("Rule[%d]: keep '%s/%s' (container)", pcr.ID, p.Name, pv.Version)
78-
continue
79-
}
80-
}
87+
func executeCleanupOneRule(ctx context.Context, pcr *packages_model.PackageCleanupRule) error {
88+
if err := pcr.CompiledPattern(); err != nil {
89+
return fmt.Errorf("CleanupRule [%d]: CompilePattern failed: %w", pcr.ID, err)
90+
}
8191

82-
toMatch := pv.LowerVersion
83-
if pcr.MatchFullName {
84-
toMatch = p.LowerName + "/" + pv.LowerVersion
85-
}
92+
packages, err := packages_model.GetPackagesByType(ctx, pcr.OwnerID, pcr.Type)
93+
if err != nil {
94+
return fmt.Errorf("CleanupRule [%d]: GetPackagesByType failed: %w", pcr.ID, err)
95+
}
8696

87-
if pcr.KeepPatternMatcher != nil && pcr.KeepPatternMatcher.MatchString(toMatch) {
88-
log.Debug("Rule[%d]: keep '%s/%s' (keep pattern)", pcr.ID, p.Name, pv.Version)
89-
continue
90-
}
91-
if pv.CreatedUnix.AsLocalTime().After(olderThan) {
92-
log.Debug("Rule[%d]: keep '%s/%s' (remove days)", pcr.ID, p.Name, pv.Version)
93-
continue
94-
}
95-
if pcr.RemovePatternMatcher != nil && !pcr.RemovePatternMatcher.MatchString(toMatch) {
96-
log.Debug("Rule[%d]: keep '%s/%s' (remove pattern)", pcr.ID, p.Name, pv.Version)
97-
continue
97+
anyVersionDeleted := false
98+
for _, p := range packages {
99+
versionDeleted := false
100+
err = db.WithTx(ctx, func(ctx context.Context) (err error) {
101+
versionDeleted, err = executeCleanupOneRulePackage(ctx, pcr, p)
102+
return err
103+
})
104+
if err != nil {
105+
log.Error("CleanupRule [%d]: executeCleanupOneRulePackage(%d) failed: %v", pcr.ID, p.ID, err)
106+
continue
107+
}
108+
anyVersionDeleted = anyVersionDeleted || versionDeleted
109+
if versionDeleted {
110+
if pcr.Type == packages_model.TypeCargo {
111+
owner, err := user_model.GetUserByID(ctx, pcr.OwnerID)
112+
if err != nil {
113+
return fmt.Errorf("GetUserByID failed: %w", err)
98114
}
99-
100-
log.Debug("Rule[%d]: remove '%s/%s'", pcr.ID, p.Name, pv.Version)
101-
102-
if err := packages_service.DeletePackageVersionAndReferences(ctx, pv); err != nil {
103-
return fmt.Errorf("CleanupRule [%d]: DeletePackageVersionAndReferences failed: %w", pcr.ID, err)
115+
if err := cargo_service.UpdatePackageIndexIfExists(ctx, owner, owner, p.ID); err != nil {
116+
return fmt.Errorf("CleanupRule [%d]: cargo.UpdatePackageIndexIfExists failed: %w", pcr.ID, err)
104117
}
118+
}
119+
}
120+
}
105121

106-
versionDeleted = true
107-
anyVersionDeleted = true
122+
if anyVersionDeleted {
123+
switch pcr.Type {
124+
case packages_model.TypeDebian:
125+
if err := debian_service.BuildAllRepositoryFiles(ctx, pcr.OwnerID); err != nil {
126+
return fmt.Errorf("CleanupRule [%d]: debian.BuildAllRepositoryFiles failed: %w", pcr.ID, err)
127+
}
128+
case packages_model.TypeAlpine:
129+
if err := alpine_service.BuildAllRepositoryFiles(ctx, pcr.OwnerID); err != nil {
130+
return fmt.Errorf("CleanupRule [%d]: alpine.BuildAllRepositoryFiles failed: %w", pcr.ID, err)
108131
}
132+
case packages_model.TypeRpm:
133+
if err := rpm_service.BuildAllRepositoryFiles(ctx, pcr.OwnerID); err != nil {
134+
return fmt.Errorf("CleanupRule [%d]: rpm.BuildAllRepositoryFiles failed: %w", pcr.ID, err)
135+
}
136+
case packages_model.TypeArch:
137+
release, err := arch_service.AquireRegistryLock(ctx, pcr.OwnerID)
138+
if err != nil {
139+
return err
140+
}
141+
defer release()
109142

110-
if versionDeleted {
111-
if pcr.Type == packages_model.TypeCargo {
112-
owner, err := user_model.GetUserByID(ctx, pcr.OwnerID)
113-
if err != nil {
114-
return fmt.Errorf("GetUserByID failed: %w", err)
115-
}
116-
if err := cargo_service.UpdatePackageIndexIfExists(ctx, owner, owner, p.ID); err != nil {
117-
return fmt.Errorf("CleanupRule [%d]: cargo.UpdatePackageIndexIfExists failed: %w", pcr.ID, err)
118-
}
119-
}
143+
if err := arch_service.BuildAllRepositoryFiles(ctx, pcr.OwnerID); err != nil {
144+
return fmt.Errorf("CleanupRule [%d]: arch.BuildAllRepositoryFiles failed: %w", pcr.ID, err)
120145
}
121146
}
147+
}
148+
return nil
149+
}
122150

123-
if anyVersionDeleted {
124-
switch pcr.Type {
125-
case packages_model.TypeDebian:
126-
if err := debian_service.BuildAllRepositoryFiles(ctx, pcr.OwnerID); err != nil {
127-
return fmt.Errorf("CleanupRule [%d]: debian.BuildAllRepositoryFiles failed: %w", pcr.ID, err)
128-
}
129-
case packages_model.TypeAlpine:
130-
if err := alpine_service.BuildAllRepositoryFiles(ctx, pcr.OwnerID); err != nil {
131-
return fmt.Errorf("CleanupRule [%d]: alpine.BuildAllRepositoryFiles failed: %w", pcr.ID, err)
132-
}
133-
case packages_model.TypeRpm:
134-
if err := rpm_service.BuildAllRepositoryFiles(ctx, pcr.OwnerID); err != nil {
135-
return fmt.Errorf("CleanupRule [%d]: rpm.BuildAllRepositoryFiles failed: %w", pcr.ID, err)
136-
}
137-
case packages_model.TypeArch:
138-
release, err := arch_service.AquireRegistryLock(ctx, pcr.OwnerID)
139-
if err != nil {
140-
return err
141-
}
142-
defer release()
151+
func ExecuteCleanupRules(ctx context.Context) error {
152+
return packages_model.IterateEnabledCleanupRules(ctx, func(ctx context.Context, pcr *packages_model.PackageCleanupRule) error {
153+
select {
154+
case <-ctx.Done():
155+
return db.ErrCancelledf("While processing package cleanup rules")
156+
default:
157+
}
143158

144-
if err := arch_service.BuildAllRepositoryFiles(ctx, pcr.OwnerID); err != nil {
145-
return fmt.Errorf("CleanupRule [%d]: arch.BuildAllRepositoryFiles failed: %w", pcr.ID, err)
146-
}
147-
}
159+
err := executeCleanupOneRule(ctx, pcr)
160+
if err != nil {
161+
log.Error("CleanupRule [%d]: executeCleanupOneRule failed: %v", pcr.ID, err)
148162
}
149163
return nil
150164
})
151-
if err != nil {
152-
return err
153-
}
154-
155-
return committer.Commit()
156165
}
157166

158167
func CleanupExpiredData(outerCtx context.Context, olderThan time.Duration) error {

‎tests/integration/api_packages_test.go

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -636,12 +636,16 @@ func TestPackageCleanup(t *testing.T) {
636636
},
637637
{
638638
Name: "Mixed",
639-
Versions: []version{
640-
{Version: "keep", ShouldExist: true, Created: time.Now().Add(time.Duration(10000)).Unix()},
641-
{Version: "dummy", ShouldExist: true, Created: 1},
642-
{Version: "test-3", ShouldExist: true},
643-
{Version: "test-4", ShouldExist: false, Created: 1},
644-
},
639+
Versions: func(limit, removeDays int) []version {
640+
aa := []version{
641+
{Version: "keep", ShouldExist: true, Created: time.Now().Add(time.Duration(10000)).Unix()},
642+
{Version: "dummy", ShouldExist: true, Created: 1},
643+
}
644+
for i := range limit {
645+
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()})
646+
}
647+
return aa
648+
}(220, 7),
645649
Rule: &packages_model.PackageCleanupRule{
646650
Enabled: true,
647651
KeepCount: 1,
@@ -686,7 +690,7 @@ func TestPackageCleanup(t *testing.T) {
686690
err = packages_service.DeletePackageVersionAndReferences(db.DefaultContext, pv)
687691
assert.NoError(t, err)
688692
} else {
689-
assert.ErrorIs(t, err, packages_model.ErrPackageNotExist)
693+
assert.ErrorIs(t, err, packages_model.ErrPackageNotExist, v.Version)
690694
}
691695
}
692696

0 commit comments

Comments
 (0)