From 33b90de63d5f76e8e933d110fb3f8ec6e28f3f64 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Batistoni?= Date: Fri, 17 Jul 2026 13:38:25 -0300 Subject: [PATCH] Fix CreatePipeline retry (#6859) --- server/store/datastore/helper.go | 7 +++++-- server/store/datastore/helper_test.go | 16 ++++++++++++++++ server/store/datastore/pipeline.go | 22 ++++------------------ server/store/datastore/pipeline_test.go | 25 +++++++++++++++++++++++++ 4 files changed, 50 insertions(+), 20 deletions(-) diff --git a/server/store/datastore/helper.go b/server/store/datastore/helper.go index 9b04088de..1678aa6fb 100644 --- a/server/store/datastore/helper.go +++ b/server/store/datastore/helper.go @@ -53,8 +53,11 @@ func wrapDelete(c int64, err error) error { func wrapInsert(c int64, err error) error { if err != nil { - if errMsg := err.Error(); strings.HasPrefix(errMsg, "UNIQUE constraint failed") || - strings.HasPrefix(errMsg, "pq: duplicate key value violates unique constraint") || + // Common unique constraint violation patterns across the supported drivers. + if errMsg := err.Error(); strings.Contains(errMsg, "UNIQUE constraint failed") || + strings.Contains(errMsg, "UNIQUE violation") || + strings.Contains(errMsg, "duplicate key") || + strings.Contains(errMsg, "unique constraint") || strings.Contains(errMsg, "Duplicate entry") { return types.ErrInsertDuplicateDetected } diff --git a/server/store/datastore/helper_test.go b/server/store/datastore/helper_test.go index d7f84f48e..c876dd065 100644 --- a/server/store/datastore/helper_test.go +++ b/server/store/datastore/helper_test.go @@ -50,4 +50,20 @@ func TestWrapInsert(t *testing.T) { // test insert witch should fail because of unique constraint assert.ErrorIs(t, wrapInsert(store.engine.Insert(cron)), types.ErrInsertDuplicateDetected) + + // The store above only exercises the sqlite wording. Cover the other drivers too: callers rely on + // errors.Is(err, ErrInsertDuplicateDetected) instead of matching driver strings themselves, so a + // pattern missing here silently breaks them (e.g. the retry in CreatePipeline, #6067). + for _, driverErr := range []string{ + `pq: duplicate key value violates unique constraint "UQE_pipelines_s"`, // postgres + "Error 1062: Duplicate entry '1-2' for key 'UQE_pipelines_s'", // mysql + "UNIQUE violation", + "unique constraint", + } { + assert.ErrorIs(t, wrapInsert(0, errors.New(driverErr)), types.ErrInsertDuplicateDetected, driverErr) + } + + // Everything else passes through untouched. + other := errors.New("connection refused") + assert.Equal(t, other, wrapInsert(0, other)) } diff --git a/server/store/datastore/pipeline.go b/server/store/datastore/pipeline.go index 1a586866e..d5672fdb1 100644 --- a/server/store/datastore/pipeline.go +++ b/server/store/datastore/pipeline.go @@ -16,7 +16,7 @@ package datastore import ( "context" - "strings" + "errors" "time" "github.com/cenkalti/backoff/v7" @@ -24,6 +24,7 @@ import ( "xorm.io/xorm" "go.woodpecker-ci.org/woodpecker/v3/server/model" + "go.woodpecker-ci.org/woodpecker/v3/server/store/types" ) func (s storage) GetPipeline(id int64) (*model.Pipeline, error) { @@ -169,7 +170,7 @@ func (s storage) CreatePipeline(pipeline *model.Pipeline, stepList ...*model.Ste pipeline.Created = time.Now().UTC().Unix() // only Insert set auto created ID back to object if err := wrapInsert(sess.Insert(pipeline)); err != nil { - if isUniqueConstraintError(err) { + if errors.Is(err, types.ErrInsertDuplicateDetected) { return struct{}{}, err } return struct{}{}, backoff.Permanent(err) @@ -179,7 +180,7 @@ func (s storage) CreatePipeline(pipeline *model.Pipeline, stepList ...*model.Ste stepList[i].PipelineID = pipeline.ID // only Insert set auto created ID back to object if err := wrapInsert(sess.Insert(stepList[i])); err != nil { - if isUniqueConstraintError(err) { + if errors.Is(err, types.ErrInsertDuplicateDetected) { return struct{}{}, err } return struct{}{}, backoff.Permanent(err) @@ -192,21 +193,6 @@ func (s storage) CreatePipeline(pipeline *model.Pipeline, stepList ...*model.Ste return err } -// isUniqueConstraintError checks if an error is a unique constraint violation error. -func isUniqueConstraintError(err error) bool { - if err == nil { - return false - } - - errStr := err.Error() - // Check for common unique constraint error patterns across different databases - return strings.Contains(errStr, "duplicate key") || - strings.Contains(errStr, "Duplicate entry") || - strings.Contains(errStr, "UNIQUE constraint failed") || - strings.Contains(errStr, "unique constraint") || - strings.Contains(errStr, "UNIQUE violation") -} - func (s storage) UpdatePipeline(pipeline *model.Pipeline) error { _, err := s.engine.ID(pipeline.ID).AllCols().Update(pipeline) return err diff --git a/server/store/datastore/pipeline_test.go b/server/store/datastore/pipeline_test.go index b5c0e9a33..1126d557b 100644 --- a/server/store/datastore/pipeline_test.go +++ b/server/store/datastore/pipeline_test.go @@ -218,6 +218,31 @@ func TestPipelineIncrement(t *testing.T) { assert.EqualValues(t, 1, pipelineC.Number) } +// Duplicates must surface as ErrInsertDuplicateDetected so the retry sees them as retryable (#6067). +func TestCreatePipelineDuplicateIsRetryable(t *testing.T) { + store, closer := newTestStore(t, new(model.Repo), new(model.Step), new(model.Pipeline)) + defer closer() + + require.NoError(t, store.CreateRepo(&model.Repo{ID: 1, Owner: "1", Name: "1", FullName: "1/1", ForgeRemoteID: "1"})) + + // Duplicate step: UNIQUE(pipeline_id, pid). + err := store.CreatePipeline(&model.Pipeline{RepoID: 1}, + &model.Step{PID: 1, Name: "clone"}, + &model.Step{PID: 1, Name: "dup"}, + ) + assert.ErrorIs(t, err, types.ErrInsertDuplicateDetected) + + // Every attempt rolled back. + count, err := store.GetPipelineCount() + assert.NoError(t, err) + assert.Zero(t, count) + + // Same for the pipeline insert. Forced through the PK: the number is MAX+1, it can't collide on demand. + existing := &model.Pipeline{RepoID: 1} + require.NoError(t, store.CreatePipeline(existing)) + assert.ErrorIs(t, store.CreatePipeline(&model.Pipeline{ID: existing.ID, RepoID: 1}), types.ErrInsertDuplicateDetected) +} + func TestDeletePipeline(t *testing.T) { store, closer := newTestStore(t, new(model.Pipeline), new(model.Repo), new(model.Workflow), new(model.Step), new(model.LogEntry), new(model.PipelineConfig), new(model.Config))