[tempel-it]

Summary

chpool delegates pooling to jackc/puddle/v2, which stores idle connections in a LIFO stack: Acquire always returns the most recently released connection. When pool connections land on heterogeneous backends behind a load balancer (e.g. replicas with unequal speed), this creates a feedback loop: the connection to the slowest replica finishes its query last, is released last, lands on top of the stack, and is reused first. The slow replica attracts a disproportionate share of queries, gets slower under the extra load, and the loop locks in, while per-replica connection counts look perfectly even.

This PR adds a chpool.Options.AcquireStrategy option:

Strategy	Behavior	Best for
AcquireLIFO (default, zero value)	Returns the most recently released connection, keeping a small set warm and letting idle ones age out via MaxConnIdleTime.	Homogeneous backends; unchanged current behavior.
AcquireFIFO	Returns the least recently released connection, rotating through all idle connections and spreading queries evenly across backends.	Heterogeneous backends behind a load balancer.

AcquireFIFO trade-offs (also documented on the constant): all idle connections stay in rotation, so the pool does not shrink via MaxConnIdleTime while it stays under load; each acquire reorders the idle set, which serializes concurrent acquires and costs O(n log n) in the number of idle connections; ordering is best-effort when acquires race; Stat's EmptyAcquireCount/EmptyAcquireWaitTime may be inflated because the reorder briefly drains the idle set.

Default behavior is unchanged: AcquireLIFO is the zero value, and on that path Acquire is identical to today apart from one branch check. All existing tests pass as-is.

Design

puddle does not expose an acquisition-order option and its idle store is an internal LIFO stack, so the options were:

• Reorder before acquire (chosen). Uses only public puddle APIs: AcquireAllIdle() → stable sort newest-first by LastUsedNanotime() → ReleaseUnused() each, so the oldest connection ends up on top and the following Acquire pops it. ReleaseUnused preserves LastUsedNanotime, so MaxConnIdleTime enforcement and AcquireCount statistics are unaffected. A mutex serializes reorders so two concurrent ones cannot split the idle set and re-push it interleaved.
• Shadow-track idle connections in chpool. Rejected: connections held outside puddle are invisible to its health check (AcquireAllIdle) and break MaxConns wakeup accounting.
• Replace puddle with an internal pool. Rejected: Pool.Stat() exposes *puddle.Stat (unexported fields, no constructor), so removal is a breaking API change, and a rewrite carries far more behavioral risk than a reorder layered on public APIs.

Possible follow-up (not in this PR): skip the reorder when no release happened since the previous one, so bursts of acquires pay the sorting cost once. Longer term, an acquisition-order option in puddle itself would be the cleaner home; if that lands upstream, this reordering can be removed without changing chpool's API.

Tests

• TestNew_InvalidAcquireStrategy: unknown strategy values are rejected.
• TestPool_AcquireStrategyOrder: deterministic proof that FIFO re-acquires N released connections in release order and LIFO (default) in reverse release order. Single goroutine, no sleeps, no timing.
• TestPool_AcquireStrategyLoadSpread: deterministic simulation of a slow backend (its connection is released last each round). LIFO acquires the slow connection first in every round and starves the remaining connections; FIFO rotates through all of them with a derived minimum share per connection.

Checklist

• Unit and integration tests covering the common scenarios were added
• A human-readable description of the changes was provided to include in CHANGELOG

CHANGELOG: Added chpool.Options.AcquireStrategy (AcquireLIFO default, AcquireFIFO) to control which idle connection Acquire returns; FIFO rotates through idle connections to spread load evenly across heterogeneous backends behind a load balancer.

🤖 Generated with Claude Code

[kavirajk]

The comment in rotateIdleToFIFO says that an extra dial can occur, and that the order self-corrects. I think that the extra dial causes problems that do not self-correct?

1. rotateIdleToFIFO sorts the idle set and releases fifoMu when it returns.
2. Then Acquire calls pool.Acquire.

Between step 1 and step 2, a different FIFO acquire (or the health check) can remove all idle connections with AcquireAllIdle. Then pool.Acquire finds no idle connection. If the pool has fewer than MaxConns connections, it dials.

This has two effects:

• Errors while idle connections exist: if the dial fails (short deadline, LB rejects new connections, server connection limit), Acquire returns an error, but healthy idle connections are available.
• The pool does not shrink: each race can add one connection. FIFO keeps all connections in rotation, so MaxConnIdleTime does not close the extra connections while the pool has a steady load I think?.

Test that reproduces the problem for example

func TestPool_AcquireStrategyNoExtraDial(t *testing.T) {
	t.Parallel()

	const (
		idle       = 4
		maxConns   = 10
		workers    = 2 // Less than idle. Thus, an idle connection is always available.
		iterations = 5000
	)

	run := func(t *testing.T, strategy AcquireStrategy) {
		p := PoolConnOpt(t, Options{
			MaxConns:          maxConns,
			AcquireStrategy:   strategy,
			HealthCheckPeriod: time.Hour,
		})

		warm := make([]*Client, 0, idle)
		for i := 0; i < idle; i++ {
			c, err := p.Acquire(context.Background())
			require.NoError(t, err)
			warm = append(warm, c)
		}
		for _, c := range warm {
			c.Release()
		}
		require.EqualValues(t, idle, p.Stat().IdleResources())

		// Make each new dial fail quickly. The constructor reads
		// p.options when it dials. The test writes p.options before
		// the workers start.
		ln, err := net.Listen("tcp", "127.0.0.1:0")
		require.NoError(t, err)
		closedAddr := ln.Addr().String()
		require.NoError(t, ln.Close())
		p.options.ClientOptions.Address = closedAddr
		p.options.ClientOptions.DialTimeout = 100 * time.Millisecond

		var (
			wg       sync.WaitGroup
			failures atomic.Int64
			firstErr atomic.Value
		)
		for w := 0; w < workers; w++ {
			wg.Add(1)
			go func() {
				defer wg.Done()
				for i := 0; i < iterations; i++ {
					c, err := p.Acquire(context.Background())
					if err != nil {
						failures.Add(1)
						firstErr.CompareAndSwap(nil, err)
						continue
					}
					c.Release()
				}
			}()
		}
		wg.Wait()

		stat := p.Stat()
		t.Logf("failures=%d total=%d idle=%d emptyAcquires=%d",
			failures.Load(), stat.TotalResources(), stat.IdleResources(), stat.EmptyAcquireCount())
		require.Zero(t, failures.Load(),
			"Acquire dialed, but %d idle connections were available for %d workers: %v",
			idle, workers, firstErr.Load())
		require.EqualValues(t, idle, stat.TotalResources())
	}

	t.Run("LIFO", func(t *testing.T) {
		t.Parallel()
		run(t, AcquireLIFO)
	})
	t.Run("FIFO", func(t *testing.T) {
		t.Parallel()
		run(t, AcquireFIFO)
	})
}

A possible fix I think could be AcquireAllIdle returns acquired connections, so keep the oldest connection from that set and release the other connections.
Then the second pool.Acquire call is not necessary. Only fall back to pool.Acquire(ctx) when the idle set is empty.
The health check must also lock fifoMu in FIFO mode. This also changes the cost from O(n log n) to O(n), because a sort is not necessary I think?.

What do you think @tempel-it ?

[kavirajk]

@tempel-it ping about the PR remarks