upstream/forgejo
1
0
Fork 0
mirror of https://codeberg.org/forgejo/forgejo.git synced 2026-05-28 11:14:54 -04:00
Code Issues Projects Releases Packages Wiki Activity Actions 4

[v15.0/forgejo] fix: debian package cleanup failure due to xorm connection corruption (#12774)

Browse source 
**Backport:** https://codeberg.org/forgejo/forgejo/pulls/12764

Fixes #12645.  Detailed analysis in [this comment](https://codeberg.org/forgejo/forgejo/issues/12645#issuecomment-15939122).  New test case is verified to hit the bug -- the previous case just narrowly missed the problem because it ended up with an empty repository index.

Co-authored-by: Mathieu Fenniak <mathieu@fenniak.net>
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/12774
Reviewed-by: Gusted <gusted@noreply.codeberg.org>
This commit is contained in:
forgejo-backport-action 2026-05-27 19:31:03 +02:00 committed by Gusted
parent a08bca648c
commit 76b6d34add
2 changed files with 76 additions and 28 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

33
models/packages/debian/search.go
View file

@ -76,22 +76,35 @@ func ExistPackages(ctx context.Context, opts *PackageSearchOptions) (bool, error
// SearchPackages gets the packages matching the search options
func SearchPackages(ctx context.Context, opts *PackageSearchOptions, iter func(*packages.PackageFileDescriptor)) error {
return db.GetEngine(ctx).
// Forgejo's db library doesn't have an iterate method that allows us to do this query, *sorted*, in chunks. xorm's
// `Iterate` isn't usable because sometimes we are in a transaction, and GetPackageFileDescriptor will make
// supplemental DB calls which will fail in xorm's Iterate as the same connection will come back from
// db.GetEngine(ctx) due to the transaction, and that connection is already being used for the iterate.
//
// Aside from the reasons we can't do this iteratively, `SearchPackages` is only used when we're rebuilding the
// package index, and the caller `buildPackagesIndices` is already building *three* in-memory files referencing all
// this same information -- so the memory usage to load this information in one chunk shouldn't be significantly
// worse than the index rebuild side anyway.
pfs := make([]*packages.PackageFile, 0, 100)
err := db.GetEngine(ctx).
Table("package_file").
Select("package_file.*").
Join("INNER", "package_version", "package_version.id = package_file.version_id").
Join("INNER", "package", "package.id = package_version.package_id").
Where(opts.toCond()).
Asc("package.lower_name", "package_version.created_unix").
Iterate(&packages.PackageFile{}, func(i int, bean any) error {
pf := bean.(*packages.PackageFile)
pfd, err := packages.GetPackageFileDescriptor(ctx, pf)
if err != nil {
return err
}
iter(pfd)
return nil
})
Find(&pfs)
if err != nil {
return err
}
for _, pf := range pfs {
pfd, err := packages.GetPackageFileDescriptor(ctx, pf)
if err != nil {
return err
}
iter(pfd)
}
return nil
}
// GetDistributions gets all available distributions

71
tests/integration/api_packages_test.go
View file

@ -493,9 +493,7 @@ func TestPackageCleanup(t *testing.T) {
duration, _ := time.ParseDuration("-1h")
t.Run("Debian", func(t *testing.T) {
defer tests.PrintCurrentTest(t)()
// Debian does a repository rebuild.
// Debian does a repository rebuild; these tests cover validation of that process.
distribution := "forgejo"
component := "main"
architecture := "amd64"
@ -505,27 +503,64 @@ func TestPackageCleanup(t *testing.T) {
rootURL := fmt.Sprintf("/api/packages/%s/debian", user.Name)
uploadURL := fmt.Sprintf("%s/pool/%s/%s/upload", rootURL, distribution, component)
req := NewRequestWithBody(t, "PUT", uploadURL,
createDebianArchive(packageName, "1.0.0", architecture, packageDescription)).
AddBasicAuth(user.Name)
MakeRequest(t, req, http.StatusCreated)
t.Run("empty repository after cleanup", func(t *testing.T) {
defer tests.PrintCurrentTest(t)()
resp := MakeRequest(t, NewRequestf(t, "GET", "%s/dists/%s/%s/binary-%s/Packages", rootURL, distribution, component, architecture), http.StatusOK)
assert.Contains(t, resp.Body.String(), "pool/forgejo/main/runner_1.0.0_amd64.deb")
req := NewRequestWithBody(t, "PUT", uploadURL,
createDebianArchive(packageName, "1.0.0", architecture, packageDescription)).
AddBasicAuth(user.Name)
MakeRequest(t, req, http.StatusCreated)
pcr, err := packages_model.InsertCleanupRule(t.Context(), &packages_model.PackageCleanupRule{
Enabled: true,
RemovePattern: `.+`,
OwnerID: user.ID,
Type: packages_model.TypeDebian,
resp := MakeRequest(t, NewRequestf(t, "GET", "%s/dists/%s/%s/binary-%s/Packages", rootURL, distribution, component, architecture), http.StatusOK)
assert.Contains(t, resp.Body.String(), "pool/forgejo/main/runner_1.0.0_amd64.deb")
pcr, err := packages_model.InsertCleanupRule(t.Context(), &packages_model.PackageCleanupRule{
Enabled: true,
RemovePattern: `.+`,
OwnerID: user.ID,
Type: packages_model.TypeDebian,
})
require.NoError(t, err)
require.NoError(t, packages_cleanup_service.CleanupTask(t.Context(), duration))
MakeRequest(t, NewRequestf(t, "GET", "%s/dists/%s/%s/binary-%s/Packages", rootURL, distribution, component, architecture), http.StatusNotFound)
require.NoError(t, packages_model.DeleteCleanupRuleByID(t.Context(), pcr.ID))
})
require.NoError(t, err)
require.NoError(t, packages_cleanup_service.CleanupTask(t.Context(), duration))
t.Run("non-empty repository after cleanup", func(t *testing.T) {
defer tests.PrintCurrentTest(t)()
MakeRequest(t, NewRequestf(t, "GET", "%s/dists/%s/%s/binary-%s/Packages", rootURL, distribution, component, architecture), http.StatusNotFound)
req := NewRequestWithBody(t, "PUT", uploadURL,
createDebianArchive(packageName, "1.0.0", architecture, packageDescription)).
AddBasicAuth(user.Name)
MakeRequest(t, req, http.StatusCreated)
req = NewRequestWithBody(t, "PUT", uploadURL,
createDebianArchive(packageName, "1.0.1", architecture, packageDescription)).
AddBasicAuth(user.Name)
MakeRequest(t, req, http.StatusCreated)
require.NoError(t, packages_model.DeleteCleanupRuleByID(t.Context(), pcr.ID))
resp := MakeRequest(t, NewRequestf(t, "GET", "%s/dists/%s/%s/binary-%s/Packages", rootURL, distribution, component, architecture), http.StatusOK)
assert.Contains(t, resp.Body.String(), "pool/forgejo/main/runner_1.0.0_amd64.deb")
assert.Contains(t, resp.Body.String(), "pool/forgejo/main/runner_1.0.1_amd64.deb")
pcr, err := packages_model.InsertCleanupRule(t.Context(), &packages_model.PackageCleanupRule{
Enabled: true,
RemovePattern: `.+`,
OwnerID: user.ID,
Type: packages_model.TypeDebian,
KeepCount: 1,
})
require.NoError(t, err)
require.NoError(t, packages_cleanup_service.CleanupTask(t.Context(), duration))
resp = MakeRequest(t, NewRequestf(t, "GET", "%s/dists/%s/%s/binary-%s/Packages", rootURL, distribution, component, architecture), http.StatusOK)
assert.Contains(t, resp.Body.String(), "pool/forgejo/main/runner_1.0.1_amd64.deb")
require.NoError(t, packages_model.DeleteCleanupRuleByID(t.Context(), pcr.ID))
})
})
t.Run("Common", func(t *testing.T) {