mirror of
https://codeberg.org/forgejo/forgejo.git
synced 2026-05-12 22:10:25 +00:00
test: fix intermittent test failure in TestPackageDebianConcurrent (#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. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/11997 Reviewed-by: Andreas Ahlenstorf <aahlenst@noreply.codeberg.org> Co-authored-by: Mathieu Fenniak <mathieu@fenniak.net> Co-committed-by: Mathieu Fenniak <mathieu@fenniak.net>
This commit is contained in:
parent
15b4c5efe8
commit
6a879e79df
2 changed files with 16 additions and 12 deletions
|
|
@ -318,9 +318,10 @@ func buildReleaseFiles(ctx context.Context, ownerID int64, repoVersion *packages
|
||||||
// way.
|
// way.
|
||||||
var priv string
|
var priv string
|
||||||
err = db.RetryTx(ctx, db.RetryConfig{
|
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.
|
// A single retry is sufficient the user/org's key pair would have been created by the first successful tx; an
|
||||||
AttemptCount: 2,
|
// additional retry may be necessary if a deadlock occurs with concurrent updates.
|
||||||
ErrorIs: []error{xorm.ErrUniqueConstraintViolation},
|
AttemptCount: 3,
|
||||||
|
ErrorIs: []error{xorm.ErrUniqueConstraintViolation, xorm.ErrDeadlock},
|
||||||
}, func(ctx context.Context) error {
|
}, func(ctx context.Context) error {
|
||||||
priv, _, err = GetOrCreateKeyPair(ctx, ownerID)
|
priv, _, err = GetOrCreateKeyPair(ctx, ownerID)
|
||||||
return err
|
return err
|
||||||
|
|
|
||||||
|
|
@ -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
|
// causes such a recovery from error to panic. So, we retry the entire modification transaction if
|
||||||
// ErrUniqueConstraintViolation is encountered.
|
// ErrUniqueConstraintViolation is encountered.
|
||||||
err := db.RetryTx(ctx, db.RetryConfig{
|
err := db.RetryTx(ctx, db.RetryConfig{
|
||||||
// A single retry is sufficient as any package index that was concurrently modified should now be present:
|
// A single retry is sufficient as any package index that was concurrently modified should now be present; an
|
||||||
AttemptCount: 2,
|
// additional retry may be necessary if a deadlock occurs with concurrent updates.
|
||||||
ErrorIs: []error{xorm.ErrUniqueConstraintViolation},
|
AttemptCount: 3,
|
||||||
|
ErrorIs: []error{xorm.ErrUniqueConstraintViolation, xorm.ErrDeadlock},
|
||||||
}, func(ctx context.Context) error {
|
}, func(ctx context.Context) error {
|
||||||
var err error
|
var err error
|
||||||
var pb *packages_model.PackageBlob
|
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.
|
// See comment in createPackageAndAddFile which explains why RetryTx is used with ErrUniqueConstraintViolation.
|
||||||
err := db.RetryTx(ctx, db.RetryConfig{
|
err := db.RetryTx(ctx, db.RetryConfig{
|
||||||
// A single retry is sufficient as any package index that was concurrently modified should now be present:
|
// A single retry is sufficient as any package index that was concurrently modified should now be present; an
|
||||||
AttemptCount: 2,
|
// additional retry may be necessary if a deadlock occurs with concurrent updates.
|
||||||
ErrorIs: []error{xorm.ErrUniqueConstraintViolation},
|
AttemptCount: 3,
|
||||||
|
ErrorIs: []error{xorm.ErrUniqueConstraintViolation, xorm.ErrDeadlock},
|
||||||
}, func(ctx context.Context) error {
|
}, func(ctx context.Context) error {
|
||||||
var err error
|
var err error
|
||||||
var blobCreated bool
|
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.
|
// See comment in createPackageAndAddFile which explains why RetryTx is used with ErrUniqueConstraintViolation.
|
||||||
return pv, db.RetryTx(ctx, db.RetryConfig{
|
return pv, db.RetryTx(ctx, db.RetryConfig{
|
||||||
// A single retry is sufficient as any package index that was concurrently modified should now be present:
|
// A single retry is sufficient as any package index that was concurrently modified should now be present; an
|
||||||
AttemptCount: 2,
|
// additional retry may be necessary if a deadlock occurs with concurrent updates.
|
||||||
ErrorIs: []error{xorm.ErrUniqueConstraintViolation},
|
AttemptCount: 3,
|
||||||
|
ErrorIs: []error{xorm.ErrUniqueConstraintViolation, xorm.ErrDeadlock},
|
||||||
}, func(ctx context.Context) error {
|
}, func(ctx context.Context) error {
|
||||||
p := &packages_model.Package{
|
p := &packages_model.Package{
|
||||||
OwnerID: ownerID,
|
OwnerID: ownerID,
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue