Skip to content

Commit 9ccd941

Browse files
marioneblclaude
andcommitted
fix(reposerver): address PR review feedback for convoy deadlock fix
- Move convoy tests from lock_convoy_test.go into lock_test.go - Add early context cancellation check at start of Lock() Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 979fd5c commit 9ccd941

File tree

3 files changed

+60
-69
lines changed

3 files changed

+60
-69
lines changed

reposerver/repository/lock.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,9 @@ type repositoryLock struct {
2424
// The init callback receives `clean` parameter which indicates if repo state must be cleaned after running non-concurrent operation.
2525
// The first init always runs with `clean` set to true because we cannot be sure about initial repo state.
2626
func (r *repositoryLock) Lock(ctx context.Context, path string, revision string, allowConcurrent bool, init func(clean bool) (io.Closer, error)) (io.Closer, error) {
27+
if ctx.Err() != nil {
28+
return nil, ctx.Err()
29+
}
2730
r.lock.Lock()
2831
state, ok := r.stateByKey[path]
2932
if !ok {

reposerver/repository/lock_convoy_test.go

Lines changed: 0 additions & 69 deletions
This file was deleted.

reposerver/repository/lock_test.go

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,12 @@ import (
44
"context"
55
"errors"
66
"io"
7+
"sync"
78
"testing"
89
"time"
910

1011
"github.com/stretchr/testify/assert"
12+
"github.com/stretchr/testify/require"
1113

1214
utilio "github.com/argoproj/argo-cd/v3/util/io"
1315
)
@@ -145,6 +147,61 @@ func TestLock_FailedInitialization(t *testing.T) {
145147
utilio.Close(closer2)
146148
}
147149

150+
func TestLock_WaiterForDifferentRevision_CannotBeUnblocked(t *testing.T) {
151+
lock := NewRepositoryLock()
152+
init := func(_ bool) (io.Closer, error) {
153+
return utilio.NopCloser, nil
154+
}
155+
156+
// Acquire lock for revision "1"
157+
closer1, err := lock.Lock(context.Background(), "myRepo", "1", true, init)
158+
require.NoError(t, err)
159+
160+
// Try to acquire lock for revision "2" with a short timeout
161+
ctx, cancel := context.WithTimeout(context.Background(), 200*time.Millisecond)
162+
defer cancel()
163+
164+
_, err = lock.Lock(ctx, "myRepo", "2", true, init)
165+
assert.ErrorIs(t, err, context.DeadlineExceeded)
166+
167+
utilio.Close(closer1)
168+
}
169+
170+
func TestLock_ConvoyFormsUnderSequentialRevisions(t *testing.T) {
171+
lock := NewRepositoryLock()
172+
init := func(_ bool) (io.Closer, error) {
173+
return utilio.NopCloser, nil
174+
}
175+
176+
// Acquire lock for revision "A" — simulates a long-running operation
177+
closerA, err := lock.Lock(context.Background(), "myRepo", "A", true, init)
178+
require.NoError(t, err)
179+
180+
// Spawn 10 goroutines all waiting for revision "B" with short deadlines
181+
const n = 10
182+
var wg sync.WaitGroup
183+
errs := make([]error, n)
184+
185+
for i := range n {
186+
wg.Add(1)
187+
go func(idx int) {
188+
defer wg.Done()
189+
ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond)
190+
defer cancel()
191+
_, errs[idx] = lock.Lock(ctx, "myRepo", "B", true, init)
192+
}(i)
193+
}
194+
195+
wg.Wait()
196+
197+
// All goroutines should have exited via context cancellation
198+
for i, err := range errs {
199+
assert.ErrorIs(t, err, context.DeadlineExceeded, "goroutine %d should have been cancelled", i)
200+
}
201+
202+
utilio.Close(closerA)
203+
}
204+
148205
func TestLock_SameRevisionFirstNotConcurrent(t *testing.T) {
149206
lock := NewRepositoryLock()
150207
initializedTimes := 0

0 commit comments

Comments
 (0)