Skip to content

Commit 268fa12

Browse files
committed
Mark singleServerPool's connection lcActive at construction
singleServerPool's OnSuccess/OnFailure are no-ops, so a connection placed in this pool would otherwise stay at its initial lifecycle (typically lcUnknown / "dead") for the lifetime of the pool. Anything that reads lifecycle state via buildConnectionMetric -- the metrics API, test readiness gates that lens connection health into the readiness FSM, future observers -- would then incorrectly classify the only available connection as not-ready, even after it had served requests successfully. multiServerPool already does the right thing: when it constructs a pool with initial conns it CAS's lcActive on each one (clearing lcUnknown|lcStandby). This change applies the same transition to singleServerPool by routing all four construction sites through a new newSingleServerPool(conn, *metrics) helper that performs the CAS under conn.mu before returning. Behavior is otherwise unchanged. Existing multiServerPool-> singleServerPool demotion paths preserve their connection's existing state (the CAS is a no-op when lcActive is already set). Signed-off-by: Sean Chittenden <sean.chittenden@crowdstrike.com>
1 parent 8576815 commit 268fa12

4 files changed

Lines changed: 47 additions & 11 deletions

File tree

opensearchtransport/discovery.go

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -871,10 +871,7 @@ func (c *Client) createOrUpdateSingleNodePool(readyConnections, deadConnections
871871
metrics = existingSinglePool.metrics
872872
}
873873

874-
return &singleServerPool{
875-
connection: connection,
876-
metrics: metrics,
877-
}
874+
return newSingleServerPool(connection, metrics)
878875
}
879876

880877
// createOrUpdateMultiNodePoolWithLock handles multi-node connection pool creation/updates.

opensearchtransport/opensearchtransport.go

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -890,7 +890,7 @@ func New(cfg Config) (*Client, error) {
890890
} else {
891891
// Use client-configured timeout settings for the main connection pool
892892
if len(conns) == 1 {
893-
client.mu.connectionPool = &singleServerPool{connection: conns[0]}
893+
client.mu.connectionPool = newSingleServerPool(conns[0], nil)
894894
} else {
895895
poolCtx, poolCancel := context.WithCancel(ctx)
896896
pool := &multiServerPool{
@@ -2385,10 +2385,7 @@ func (c *Client) demoteConnectionPoolWithLock() *singleServerPool {
23852385

23862386
currentPool.mu.RUnlock()
23872387

2388-
return &singleServerPool{
2389-
connection: connection,
2390-
metrics: metrics,
2391-
}
2388+
return newSingleServerPool(connection, metrics)
23922389

23932390
case *singleServerPool:
23942391
// Already a singleServerPool - return unchanged

opensearchtransport/pool_multi_server.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -336,7 +336,7 @@ func (cp *multiServerPool) getWarmupParams() (int, int) {
336336
//nolint:unparam // public API; selector kept for backward compatibility
337337
func NewConnectionPool(conns []*Connection, selector Selector) ConnectionPool {
338338
if len(conns) == 1 {
339-
return &singleServerPool{connection: conns[0]}
339+
return newSingleServerPool(conns[0], nil)
340340
}
341341

342342
pool := &multiServerPool{

opensearchtransport/pool_single_server.go

Lines changed: 43 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,10 @@
66

77
package opensearchtransport
88

9-
import "net/url"
9+
import (
10+
"errors"
11+
"net/url"
12+
)
1013

1114
// singleServerPool is a trivial connection pool for single-node clusters.
1215
// All operations are no-ops except Next(), which returns the single connection.
@@ -16,6 +19,45 @@ type singleServerPool struct {
1619
metrics *metrics
1720
}
1821

22+
// newSingleServerPool constructs a singleServerPool and marks the connection
23+
// as lcActive.
24+
//
25+
// singleServerPool's OnSuccess/OnFailure are no-ops, so a connection placed
26+
// in this pool would otherwise stay at its initial lifecycle (typically
27+
// lcUnknown / "dead") for the lifetime of the pool. Observers that read
28+
// lifecycle state via buildConnectionMetric (test readiness gates, metric
29+
// consumers, etc.) would then incorrectly classify the only available
30+
// connection as not-ready.
31+
//
32+
// The transition mirrors what multiServerPool does for its initial conns:
33+
// set lcActive, clear lcUnknown|lcStandby. Construction-from-discovery
34+
// happens before the pool is published, but pool-demotion paths
35+
// (multiServerPool -> singleServerPool) reuse a connection that may
36+
// already be racing health-checkers; retry on errLifecycleConflict so a
37+
// concurrent mutation in those bits doesn't leave the connection
38+
// permanently classified as dead.
39+
func newSingleServerPool(conn *Connection, m *metrics) *singleServerPool {
40+
if conn != nil {
41+
conn.mu.Lock()
42+
const maxRetries = 4
43+
for range maxRetries {
44+
err := conn.casLifecycle(conn.loadConnState(), 0, lcActive, lcUnknown|lcStandby)
45+
if err == nil || errors.Is(err, errLifecycleNoop) {
46+
break
47+
}
48+
// errLifecycleConflict: another goroutine changed the
49+
// position bits between our snapshot and the CAS. Reload
50+
// and try again; bounded to avoid live-lock under
51+
// contention from a hot health-check goroutine.
52+
}
53+
conn.mu.Unlock()
54+
}
55+
return &singleServerPool{
56+
connection: conn,
57+
metrics: m,
58+
}
59+
}
60+
1961
// Compile-time check that singleServerPool implements ConnectionPool.
2062
var _ ConnectionPool = (*singleServerPool)(nil)
2163

0 commit comments

Comments
 (0)