fix: debian package cleanup failure due to xorm connection corruption (#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.

## Checklist

The [contributor guide](https://forgejo.org/docs/next/contributor/) contains information that will be helpful to first time contributors. All work and communication must conform to Forgejo's [AI Agreement](https://codeberg.org/forgejo/governance/src/branch/main/AIAgreement.md). There also are a few [conditions for merging Pull Requests in Forgejo repositories](https://codeberg.org/forgejo/governance/src/branch/main/PullRequestsAgreement.md). You are also welcome to join the [Forgejo development chatroom](https://matrix.to/#/#forgejo-development:matrix.org).

### Tests for Go changes

- I added test coverage for Go changes...
  - [ ] in their respective `*_test.go` for unit tests.
  - [x] in the `tests/integration` directory if it involves interactions with a live Forgejo server.
- I ran...
  - [x] `make pr-go` before pushing

### Documentation

- [ ] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change.
- [x] I did not document these changes and I do not expect someone else to do it.

### Release notes

- [x] This change will be noticed by a Forgejo user or admin (feature, bug fix, performance, etc.). I suggest to include a release note for this change.
- [ ] This change is not visible to a Forgejo user or admin (refactor, dependency upgrade, etc.). I think there is no need to add a release note for this change.

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/12764
Reviewed-by: Andreas Ahlenstorf <aahlenst@noreply.codeberg.org>

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

tests/integration/api_packages_test.go

@@ -528,9 +528,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"
@@ -540,27 +538,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) {