From 825c2a174491ec4dda4146f8c6f7b1cbff76d170 Mon Sep 17 00:00:00 2001 From: forgejo-backport-action Date: Mon, 6 Apr 2026 03:39:53 +0200 Subject: [PATCH] [v15.0/forgejo] test: fix intermittent test failure in TestPackageDebianConcurrent (#11998) **Backport:** https://codeberg.org/forgejo/forgejo/pulls/11997 Fixes #11968. Adds deadlocks to the package `RetryTx` operations, and bumps the attempt count to 3. Technically this affects production code, not just test code, but the resulting failure is only likely to occur in highly concurrent operations when uploading packages to the debian registry for the first time for a user, which is more of a test artifact than a production likelihood. Manually tested by modifying the `Makefile` to add the `-test.count=25` option to the test command. This failed consistently on my dev system before this change, failed consistently after the deadlock err was added, and then succeeded consistently (multiple runs) after both changes were combined, giving me confidence that the intermittent failure is squashed. ## 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. - Fixing a test failure, so no new tests added, but they already failed. - 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 - [ ] 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. - [x] 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. Co-authored-by: Mathieu Fenniak Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/11998 Reviewed-by: Mathieu Fenniak Co-authored-by: forgejo-backport-action Co-committed-by: forgejo-backport-action --- services/packages/debian/repository.go | 7 ++++--- services/packages/packages.go | 21 ++++++++++++--------- 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/services/packages/debian/repository.go b/services/packages/debian/repository.go index ab8c4fdc45..e84ae45ebe 100644 --- a/services/packages/debian/repository.go +++ b/services/packages/debian/repository.go @@ -318,9 +318,10 @@ func buildReleaseFiles(ctx context.Context, ownerID int64, repoVersion *packages // way. var priv string err = db.RetryTx(ctx, db.RetryConfig{ - // A single retry is sufficient the user/org's key pair would have been created by the first successful tx. - AttemptCount: 2, - ErrorIs: []error{xorm.ErrUniqueConstraintViolation}, + // A single retry is sufficient the user/org's key pair would have been created by the first successful tx; an + // additional retry may be necessary if a deadlock occurs with concurrent updates. + AttemptCount: 3, + ErrorIs: []error{xorm.ErrUniqueConstraintViolation, xorm.ErrDeadlock}, }, func(ctx context.Context) error { priv, _, err = GetOrCreateKeyPair(ctx, ownerID) return err diff --git a/services/packages/packages.go b/services/packages/packages.go index a1772cc1d9..c75b4559c8 100644 --- a/services/packages/packages.go +++ b/services/packages/packages.go @@ -95,9 +95,10 @@ func createPackageAndAddFile(ctx context.Context, pvci *PackageCreationInfo, pfc // causes such a recovery from error to panic. So, we retry the entire modification transaction if // ErrUniqueConstraintViolation is encountered. err := db.RetryTx(ctx, db.RetryConfig{ - // A single retry is sufficient as any package index that was concurrently modified should now be present: - AttemptCount: 2, - ErrorIs: []error{xorm.ErrUniqueConstraintViolation}, + // A single retry is sufficient as any package index that was concurrently modified should now be present; an + // additional retry may be necessary if a deadlock occurs with concurrent updates. + AttemptCount: 3, + ErrorIs: []error{xorm.ErrUniqueConstraintViolation, xorm.ErrDeadlock}, }, func(ctx context.Context) error { var err error var pb *packages_model.PackageBlob @@ -237,9 +238,10 @@ func addFileToPackageWrapper(ctx context.Context, fn func(ctx context.Context) ( // See comment in createPackageAndAddFile which explains why RetryTx is used with ErrUniqueConstraintViolation. err := db.RetryTx(ctx, db.RetryConfig{ - // A single retry is sufficient as any package index that was concurrently modified should now be present: - AttemptCount: 2, - ErrorIs: []error{xorm.ErrUniqueConstraintViolation}, + // A single retry is sufficient as any package index that was concurrently modified should now be present; an + // additional retry may be necessary if a deadlock occurs with concurrent updates. + AttemptCount: 3, + ErrorIs: []error{xorm.ErrUniqueConstraintViolation, xorm.ErrDeadlock}, }, func(ctx context.Context) error { var err error var blobCreated bool @@ -468,9 +470,10 @@ func GetOrCreateInternalPackageVersion(ctx context.Context, ownerID int64, packa // See comment in createPackageAndAddFile which explains why RetryTx is used with ErrUniqueConstraintViolation. return pv, db.RetryTx(ctx, db.RetryConfig{ - // A single retry is sufficient as any package index that was concurrently modified should now be present: - AttemptCount: 2, - ErrorIs: []error{xorm.ErrUniqueConstraintViolation}, + // A single retry is sufficient as any package index that was concurrently modified should now be present; an + // additional retry may be necessary if a deadlock occurs with concurrent updates. + AttemptCount: 3, + ErrorIs: []error{xorm.ErrUniqueConstraintViolation, xorm.ErrDeadlock}, }, func(ctx context.Context) error { p := &packages_model.Package{ OwnerID: ownerID,