From ba2eca83f6dd84b2e1adec76005eb90777ea4b38 Mon Sep 17 00:00:00 2001
From: fullsend-code
 <278716306+fullsend-ai-coder[bot]@users.noreply.github.com>
Date: Thu, 23 Jul 2026 04:17:10 +0000
Subject: [PATCH] fix(#5485): move RepoPool.Release channel send inside mutex

Release previously unlocked the mutex before sending the name back
on the buffered channel. This left a window where the name was in
neither the outstanding map nor the names channel. Under concurrent
acquire/release at high concurrency (e.g. GODOG_CONCURRENCY=12),
this gap could cause lost names or pool corruption.

Move the channel send inside the mutex-held region and use
defer p.mu.Unlock() for clarity. The send is guaranteed
non-blocking because the channel buffer equals pool size and the
name was removed from the channel during Acquire.

Add TestRepoPool_ConcurrentAcquireRelease_PoolIntegrity which
verifies that all names survive concurrent churn: after 20
workers cycle through a pool of 4, every name is back in the
channel, no duplicates exist, and the outstanding map is empty.

Closes #5485
---
 pkg/behaviourtest/world/pool.go      |  7 +++-
 pkg/behaviourtest/world/pool_test.go | 56 ++++++++++++++++++++++++++++
 2 files changed, 61 insertions(+), 2 deletions(-)

diff --git a/pkg/behaviourtest/world/pool.go b/pkg/behaviourtest/world/pool.go
index 43537acba..622a25c0b 100644
--- a/pkg/behaviourtest/world/pool.go
+++ b/pkg/behaviourtest/world/pool.go
@@ -53,12 +53,15 @@ func (p *RepoPool) Acquire(ctx context.Context) (string, error) {
 // error rather than allowing a panic to crash the test runner.
 func (p *RepoPool) Release(name string) error {
 	p.mu.Lock()
+	defer p.mu.Unlock()
 	if _, ok := p.outstanding[name]; !ok {
-		p.mu.Unlock()
 		return fmt.Errorf("RepoPool: releasing %q which is not an outstanding lease (possible double-release)", name)
 	}
 	delete(p.outstanding, name)
-	p.mu.Unlock()
+	// Send inside the lock: the channel buffer equals pool size and this
+	// name was removed during Acquire, so the send is guaranteed
+	// non-blocking. Holding the lock ensures the name transitions
+	// atomically from outstanding to available with no gap.
 	p.names <- name
 	return nil
 }
diff --git a/pkg/behaviourtest/world/pool_test.go b/pkg/behaviourtest/world/pool_test.go
index e23b2c4a5..c53398860 100644
--- a/pkg/behaviourtest/world/pool_test.go
+++ b/pkg/behaviourtest/world/pool_test.go
@@ -126,6 +126,62 @@ func TestRepoPool_ConcurrentAcquireRelease(t *testing.T) {
 	assert.Len(t, acquired, 10)
 }
 
+func TestRepoPool_ConcurrentAcquireRelease_PoolIntegrity(t *testing.T) {
+	const poolSize = 4
+	const workers = 20
+
+	pool, err := NewRepoPool(poolSize)
+	require.NoError(t, err)
+
+	ctx := context.Background()
+	var wg sync.WaitGroup
+
+	// Run many workers that each acquire, hold briefly, and release.
+	// Interleaved acquire/release under contention is the scenario
+	// where the inconsistency window could lose names.
+	for range workers {
+		wg.Go(func() {
+			name, err := pool.Acquire(ctx)
+			if err != nil {
+				t.Errorf("Acquire failed: %v", err)
+				return
+			}
+			time.Sleep(time.Millisecond) // simulate work
+			if err := pool.Release(name); err != nil {
+				t.Errorf("Release failed: %v", err)
+			}
+		})
+	}
+	wg.Wait()
+
+	// After all workers finish, every name must be back in the pool.
+	// Drain the channel to verify no names were lost and none duplicated.
+	seen := make(map[string]bool)
+	for range poolSize {
+		select {
+		case name := <-pool.names:
+			assert.False(t, seen[name], "duplicate name in pool: %s", name)
+			seen[name] = true
+		default:
+			t.Fatal("pool channel has fewer names than expected")
+		}
+	}
+	assert.Len(t, seen, poolSize, "expected all %d names back in pool", poolSize)
+
+	// Channel should now be empty — no extra names.
+	select {
+	case extra := <-pool.names:
+		t.Fatalf("unexpected extra name in pool: %s", extra)
+	default:
+		// expected
+	}
+
+	// Outstanding map must be empty.
+	pool.mu.Lock()
+	assert.Empty(t, pool.outstanding, "outstanding map should be empty after all releases")
+	pool.mu.Unlock()
+}
+
 func TestRepoPool_DoubleReleaseReturnsError(t *testing.T) {
 	pool, err := NewRepoPool(1)
 	require.NoError(t, err)