From 76b6d34add3398d1b3ccf79c039fcb7750f92ccf Mon Sep 17 00:00:00 2001 From: forgejo-backport-action Date: Wed, 27 May 2026 19:31:03 +0200 Subject: [PATCH] [v15.0/forgejo] fix: debian package cleanup failure due to xorm connection corruption (#12774) **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 Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/12774 Reviewed-by: Gusted --- models/packages/debian/search.go | 33 ++++++++---- tests/integration/api_packages_test.go | 71 +++++++++++++++++++------- 2 files changed, 76 insertions(+), 28 deletions(-) diff --git a/models/packages/debian/search.go b/models/packages/debian/search.go index c528a25194..f7b56f4fc2 100644 --- a/models/packages/debian/search.go +++ b/models/packages/debian/search.go @@ -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 diff --git a/tests/integration/api_packages_test.go b/tests/integration/api_packages_test.go index f4cd42784a..259df4bbad 100644 --- a/tests/integration/api_packages_test.go +++ b/tests/integration/api_packages_test.go @@ -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) {