Skip to content

Commit 683eda8

Browse files
authored
feat(bigtable): add SessionPoolImpl (two-tier pool + scaling + debug) (#20225)
## Summary Second of five PRs porting the session pool infrastructure from `feat/bigtable-sessionz-debug` (which powers the recycle-repro fleet). Adds `SessionPoolImpl`: the concrete two-tier read/write session pool for one resource. ~4000 LOC across five files plus matching tests. ## Stack - [ ] PR-1: sessionList (#20224) — per-AFE bucketing data structure. **Not yet merged.** - [x] **PR-2 (this)** — SessionPoolImpl (pool + scaling + debug + snapshot). - [ ] PR-3 — `SessionPool` / `Invoker` interfaces + `sessionClient` / `sessionTable` factory wiring. - [ ] PR-4 — Debug pages (`sessionz` / `afez` / `flightz` / `loadz` under `bigtable/debugview/`). - [ ] PR-5 — `bigtable.Client` integration + release notes. **Because PR-1 has not landed, this PR is opened against `main` and the diff includes PR-1's commits.** Once #20224 merges the base can be re-targeted (or this branch rebased) so only PR-2's own delta shows. ## What lands **SessionPoolImpl** (5 files, ~2400 LOC prod + ~2200 LOC tests): - `session_pool.go` — struct + constructor + `Invoke` + `CheckoutSession` (waiter queue, deadline propagation) + pluggable picker via the AFE picker from #20204. - `session_pool_lifecycle.go` — `SessionHooks` wiring, consecutive-failure breaker, `Close` (5-phase teardown), `WaitGoroutines` / `spawns.Wait` choreography so no session-owned goroutine outlives the pool. - `session_pool_scaling.go` — `Tick` loop, `createSession` (dial + `OpenSession` + hook registration), `pendingStarts` / `startingSessions` accounting so scale-up decisions never double-count in-flight opens. Uses the channel-pool pick hint (`ChannelPickHintInto`, added to `connpool.go`) to attribute each session to its underlying channel. - `session_pool_debug.go` — `PoolSnapshot` / slow-vRPC ring / per-close-reason counters / scaling-history buffer / `pickHistory` ring — the input to the sessionz / afez / loadz debug pages (landing in a later PR). - `session_snapshot.go` — the value-typed snapshot record the debug surface consumes; no live locks escape. **Session helpers added** (pool-facing additions to files already touched by prior PRs, kept minimal): - `Session.loops sync.WaitGroup` + `WaitGoroutines()` — pool teardown blocks on this so `readLoop` / `heartbeatLoop` and their `notifyClosed → recordClose` callback chains fully unwind before `Close` returns. Prevents session goroutines from racing metric-var writes across test boundaries. - `Session.closeErr atomic.Pointer[error]` + `setCloseErr` / `closeError` — preserves the raw `Recv` error handed to `handleClose`. Pool surfaces this on consecutive-failure breaker trips so operators see the underlying server rejection (e.g. `FailedPrecondition` when the resource is still being created) instead of only the sentinel. **Supporting additions to existing files:** - `afe_picker.go` — const `defaultAfeRandomSubsetSize = 2` (power-of-two-choices K-choice default; matches Java). - `debug_tracer.go` — three new tag constants: `tagSessionPoolCreatePanic`, `tagSessionPoolConsecutiveFailuresTripped`, `tagSessionPoolCheckoutFailedCINil`. - `connpool.go` — `ChannelPickHintInto(ctx, *atomic.Int32)` context helper. No-op when the channel pool doesn't consume the hint. ## What does NOT land yet - `SessionPool` / `Invoker` interfaces (follow-up PR alongside sessionClient / sessionTable). - `bigtable.Client` integration (PR-3+). - Debug pages under `bigtable/debugview/` (later PR). ## Test plan - [x] `go build ./...` passes. - [x] `go vet ./internal/transport/` clean. - [x] `go test ./internal/transport/ -race -count=1 -short -skip 'AfeLbSim' -timeout=180s` — passes (32s wall). ~2200 LOC of new tests across pool lifecycle, scaling, consecutive-failure breaker, AFE integration, debug surface, snapshot rendering, plus a K-choice bench. --- # Reviewer guide ## Guide 1 — mutianf (human) ### What this PR does Adds `SessionPoolImpl`, the layer that sits above the per-AFE `sessionList` shipped in #20224 and consumes it via a two-tier picker (AFE first, then a ready session in that AFE). It owns the session lifecycle (open / active / closing / close hooks), server-driven scaling via `PoolSizer`, a consecutive-failure circuit breaker, and the debug/observability surface (histograms + ring buffers) that feeds sessionz/loadz. New files: 5 source, 6 test, ~4.9k LOC. Nothing outside `session_pool*.go` / `session_snapshot*.go` is new logic — the small edits elsewhere are hook-plumbing scaffolding already vetted by the session/AFE subagent reviewers. ### Recommended read order 1. **`session_pool.go`** — start here. Struct field layout with per-field ownership comments (`:104-179`), the `waiter` FIFO shape (`:94-101`), `CheckoutSession` two-tier pick + parking (`:235-310`), `Invoke` (`:465-559`), `Stats` (`:361-397`), `UpdateConfig` (`:402-431`), `pickerFromLoadBalancing` (`:439-461`). Skim `session_pool_test.go` (28 tests) — the FIFO waiter, Stats, and UpdateConfig behaviors are all covered there. 2. **`session_pool_lifecycle.go`** — hooks (`onActive:255`, `onClosing:308`, `onClose:336`), `recordSessionClose` once-CAS on `Session.poolCloseRecorded` (`:117-130`), `Close`'s 6-phase teardown (`:154-247`), `noteAbnormalCloseIfAny` breaker (`:363-392`), the three ticker loops (`:426-538`). Skim `session_pool_lifecycle_test.go` — every hook + `Close`. 3. **`session_pool_scaling.go`** — `Tick` (`:81-162`), `createSession` worker (`:164-274`), `scalingReason` (`:278-299`), `noDeadlineButCancellableContext` (`:301-311`). Skim `session_pool_scaling_test.go` — the `scalingInProgress` gate and panic-safety are the only non-obvious contracts. 4. **`session_pool_debug.go`** — `poolMetrics` (`:36-72`), `latencyHist` log2 histogram (`:160-228`), the four ring buffers (slow-vRPC, time-series, lifetimes, pick-history), `recordPickDecision` (`:366-387`). Skim `session_pool_debug_test.go` — mostly ring-cap and rate-computation coverage. 5. **`session_snapshot.go`** — mostly type defs. Focus on `PoolSnapshot` (`:452-594`) and `LoadBalancingSnapshot` (`:414-436`) as the debug-view contract. 6. **`session_pool_consecutive_failures_test.go`** and **`session_pool_afe_test.go`** — end-to-end behavior verification; useful for confirming intent. ### Flow of events - **CheckoutSession → Invoke → release.** `CheckoutSession` (`session_pool.go:235`) opportunistically kicks Tick if `sl.ReadyCount()==0`, snapshots the picker under `p.mu`, then two-tier picks outside the lock: `ReadyAfes()` → `PickAfe` → `Checkout(afeID)` (`:259-268`). Miss → park in the FIFO waiter queue (`:286-289`), bracket `waitersCount` for the sizer (`:291,300`). `Invoke` (`:465`) checks out, runs `sh.session.Invoke`, records latencies (`:508-523`), logs a slow-vRPC row if over threshold (`:524-557`); the deferred `sh.DecOutstanding()` + `noteVRpcOutcome` (`:493-496`) hands the OK-gated latency to the per-AFE PeakEwma tracker. Session release itself is driven by `OnSlotDrained` (installed at `session_pool_scaling.go:228-231`), which returns the handle to `sessionList` and calls `signalFree` — separate from the `defer` in `Invoke`. - **Background Tick.** `startTickLoop` (`session_pool_lifecycle.go:426`) fires every 1 s → `tickOnce` debounces via `tickPending` CAS (`:447-458`) → `Tick` (`session_pool_scaling.go:81`) samples uptimes, gates on `scalingInProgress`, calls `sizer.Decide()`, and on a positive delta reserves `pendingStarts += delta` + `spawns.Add(delta)` under `p.mu` (`:131-138`) then fans out one goroutine per session. Each `createSession` acquires the budget outside `p.mu`, dials via `streamFactory`, transfers `pendingStarts → startingSessions` in one lock (`:246-249`), starts the session, and blocks on `WaitGoroutines` so it stays on `p.spawns` until the session dies. - **Abnormal close → breaker trip.** `onClose` (`session_pool_lifecycle.go:336`) CAS's `closeRecorded`, calls `noteAbnormalCloseIfAny` (`:363`), which bumps `consecutiveFailures` and stores the raw error into `lastAbnormalCloseErr`. Crossing the threshold snapshots the poison, CAS-resets the counter, and calls `drainWaitersWithErr` — waiters get `*consecutiveFailureError` wrapping the last cause (so `errors.Is(err, ErrConsecutiveFailures)` and `status.Code(err)` both still work, `:60-82`). Counter only resets in `onActive` (`:292-293`) — a successful open, not a healthy vRPC. ### Key invariants 1. **Two-tier pick, no re-entrant `p.mu`.** `CheckoutSession` reads `p.picker` under `p.mu` (`session_pool.go:249-255`) then unlocks before calling picker/sessionList. `recordPickDecision` takes `pickerName` as a **parameter** (`session_pool_debug.go:366`, `session_pool.go:260-262`) precisely because the caller already holds no lock — but any new pool method that reads `p.picker.Name()` from a hot path must not re-take `p.mu`. 2. **Waiter FIFO with `waitersCount` bracketed.** Every `PushBack` bumps `waitersCount` (`session_pool.go:291`); every wake path (`ctx.Done`, `w.ready`) decrements it (`:294,300`). `removeWaiter` (`:316`) is idempotent via `w.elem != nil`; `signalFree` and `drainWaitersWithErr` nil out `elem` under `waitersMu` (`:329-358`). `Stats().PendingCount` reads `waitersCount.Load()` — this is the sizer's queue-depth input. 3. **Close-exactly-once accounting.** `sessionsClosed` and `closesByReason` bumps are gated by `Session.poolCloseRecorded.CompareAndSwap(false, true)` inside `recordSessionClose` (`session_pool_lifecycle.go:117-130`). `sh.closingRecorded` and `sh.closeRecorded` are per-handle CAS's protecting the lifetime histogram + the `OnClose` branch. `Close`'s Phase 1 pre-flips both CAS's on every handle (`:187-193`) so a concurrent mid-flight onClosing can't double-count. 4. **Breaker resets only on `onActive`.** `consecutiveFailures.Store(0)` and `lastAbnormalCloseErr.Store(nil)` live at `session_pool_lifecycle.go:292-293`. Not on per-vRPC OK — otherwise one long-lived healthy session would mask a run of failed opens. 5. **Hot path is atomics/RLocks; debug views take snapshots.** `Stats` is the only per-request path that briefly takes `p.mu` (`session_pool.go:362`); everything else on the vRPC path is atomic. Debug snapshotters copy under lock and format after release (`session_snapshot.go:452-594`). ### What NOT to worry about - **Session / vRPC layer itself** — shipped in #20213 / #20215 (state machine, one-in-flight, PeerInfo timing, retry oracle, heartbeat). - **Per-AFE `sessionList` I1-I6** — shipped in #20224, has its own tests. - **`PoolSizer` scaling formula** — already upstream (`pool_sizer.go`); this PR only wires it and consumes `ScaleDecision`. - **AFE pickers (`SimpleAfePicker` / `LeastInFlight` / `LeastLatency`)** — already upstream (`afe_picker.go`); this PR only builds them via `pickerFromLoadBalancing`. - **`SessionThrottler` / `AdaptiveSessionThrottler`** — already upstream; this PR consumes `Acquire` / `Release` / `UpdateConfig`. - **`ClientConfigurationManager` polling** — this pool receives `UpdateConfig` calls; the polling itself is elsewhere. ### Danger zones - **Re-entrant `p.mu` on picker access.** `recordPickDecision` intentionally takes `pickerName` as a param (`session_pool_debug.go:366`). Adding a new pool method that reads `p.picker.Name()` from within a `CheckoutSession` code path is a re-entrant deadlock; pass the name in or snapshot up-front. - **`startingSessions` / `pendingStarts` accounting.** Tick reserves `pendingStarts` under `p.mu` (`session_pool_scaling.go:131-138`), `createSession`'s `reserved` defer releases it on any early return (`:172-179`), and the transfer at `:246-249` is atomic under `p.mu`. `onActive` deletes from `startingSessions` (`session_pool_lifecycle.go:265`). Any new failure branch in `createSession` must preserve the invariant `pendingStarts + len(startingSessions) + Ready = "in-flight scale-up capacity"`. - **`budget.Acquire` blocks; must run OUTSIDE `p.mu`.** Currently at `session_pool_scaling.go:181`, deliberately after the `defer reserved` block and before any `p.mu.Lock()`. Moving it under the lock deadlocks scale-up under budget exhaustion. - **Slow-vRPC is fire-and-forget.** `recordSlowVRpc` (`session_pool_debug.go:301`) appends into a mutex-guarded ring; it's on the vRPC return path but bounded by threshold gating and one small mutex. Do not add I/O, logging fanout, or channel sends here. - **`newTestPool` / bootstrap defaults.** Test helpers construct the pool without a `ClientConfigurationManager`, so the bootstrap defaults from `defaultPoolConfig()` (`session_pool.go:218-226`) are the only config those tests see. Real callers always get `UpdateConfig` synchronously on registration — verify by reading `NewSessionPoolImpl` end-to-end, not by trusting the bootstrap values. - **`Close` phase ordering.** Phase 4 (`poolCancel`) runs AFTER Phase 3 (`wg.Wait` on graceful closes) because Phase 2's `closeCtx` derives from `p.poolCtx`; swapping order strands in-flight graceful closes on a cancelled ctx. Phase 5 (`p.spawns.Wait`) blocks on every createSession goroutine's `WaitGoroutines` — that's why createSession blocks on `s.WaitGoroutines()` at `session_pool_scaling.go:272`. --- ## Guide 2 — mutianf-bot (automated reviewer) ### REAL HAZARDS to flag - **Re-entrant `p.mu` in pool methods called from `CheckoutSession`.** Anchor: `session_pool.go:235-310`. `p.mu` is dropped at `:255` before `PickAfe` / `Checkout` / `recordPickDecision` fire. Flag any newly-added helper called from that block that re-acquires `p.mu`, or any new method that reads `p.picker.Name()` without taking the name as a parameter (see the intentional parameter pattern at `session_pool_debug.go:366`). - **`budget.Acquire` under `p.mu`.** Currently correctly outside the lock at `session_pool_scaling.go:181`. `SessionThrottler.Acquire` blocks on the budget semaphore; calling it while holding `p.mu` would deadlock scale-up. Flag any code path that acquires `p.mu` before line `:181` or moves `Acquire` inside a `Lock`/`Unlock` bracket. - **`sync.Map` allocations on hit paths.** `bumpCloseReason` uses `Load` first, `LoadOrStore(k, new(atomic.Int64))` only on miss (`session_pool_lifecycle.go:102-111`) — this is the correct pattern. Flag any new `sync.Map.LoadOrStore(key, new(...))` call on a hot path that isn't gated by a preceding `Load` — that allocates on every hit. - **Waiter counter drift.** `waitersCount.Add(+1)` at `session_pool.go:291`, `Add(-1)` on both the `ctx.Done` branch (`:294`) and the `w.ready` branch (`:300`). Flag any new wake path, timeout branch, or early-return between `:291` and `:308` that doesn't decrement, and any new enqueue site that doesn't increment. Drift here corrupts the sizer's `PendingCount` input. - **Unbalanced `pendingStarts` / `startingSessions`.** Tick increments `pendingStarts` under `p.mu` at `session_pool_scaling.go:131-138`; `createSession`'s `reserved` defer at `:172-179` releases on early return; the transfer to `startingSessions` at `:246-249` is atomic; `onActive` deletes at `session_pool_lifecycle.go:265`; failed-start deletes at `session_pool_scaling.go:253-255`. Flag any new failure branch in `createSession` that returns without either the `reserved` defer or an explicit transfer/cleanup. - **Missing CAS on close-once flags.** `sessionsClosed.Add(1)` sits downstream of `s.poolCloseRecorded.CompareAndSwap(false, true)` at `session_pool_lifecycle.go:121`. Lifetime histogram sits downstream of `sh.closingRecorded.CompareAndSwap(false, true)` at `:187` and `:317`. `recordSessionClose` re-entry is dedup'd at `:190` and `:347`. Flag any new counter bump / histogram record that isn't gated by the corresponding CAS. - **Uncontrolled goroutine spawns.** Every pool-spawned goroutine must be tracked so `Close` phases 3/5/6 can drain: createSession workers use `p.spawns.Add`/`Done` under `p.mu` (`session_pool_scaling.go:137-138,149`); `spawnTickOnce` uses the same pattern (`session_pool_lifecycle.go:464-476`); the mid-Close onActive dispatch adds at `:274`. Session-owned goroutines (`readLoop`/`heartbeatLoop`) are tracked on `Session.loops` and drained via `WaitGoroutines`. Flag any bare `go func(){…}()` in these files with no WaitGroup entry. - **Hot-path lock churn.** `p.mu` is briefly held on the fast path only at `session_pool.go:239-241` and `:249-255`. `Stats` takes `p.mu` (`:362`). Flag any new `p.mu.Lock()` inserted into `Invoke` (`:465-559`) or the pre-park stretch of `CheckoutSession` outside the two existing points. ### FALSE-POSITIVE SUPPRESSION - **"Package name should be `transport`, not `internal`."** The directory is `bigtable/internal/transport/` but the `package` declaration is `package internal` (confirmed at `session_pool.go:20`). Package name is correct as-is; do NOT flag. - **Local variable shadowing an exported type** where the local name is idiomatic (e.g. `afeID` local vs `AfeID` type in `CheckoutSession`). Already resolved intentionally; do NOT re-raise variants. - **`goimports` / `gofmt` / column-alignment / trailing-newline nits.** CI (`goimports -l`, `gofmt -l`, `go vet`) already gates these. Bot echo is noise. - **Comments referencing PR #20213 / #20215 / #20224.** Stacked-PR context, not stale references; do NOT suggest removal. - **Test coverage complaints for `pool_sizer.go`, `afe_picker.go`, `session_list.go`, `session.go`, `session_vrpc.go`, `session_throttler.go`, `client_configuration_manager.go`, `default_client_config.go`.** All shipped in earlier PRs (#20213, #20215, #20224) with their own tests; out of scope here. - **"Missing error wrapping"** on internal-only calls where the caller already annotates via `fmt.Errorf("POOL %s ...: %w", ...)` or via `btopt.Debugf`. Do NOT suggest adding a second wrap. - **Retry loop / context propagation questions on `Session.Invoke`.** That's the Session layer (`session_vrpc.go`), out of scope for this PR. - **"Consider using `sync.RWMutex` instead of `sync.Mutex` on `p.mu`."** The pool holds `p.mu` for tens of nanoseconds at a time and never for read-heavy loops; the added atomic on `RLock`/`RUnlock` would cost more than it saves. Do NOT suggest. - **"Consider extracting anonymous goroutine into named function."** Style-only; do NOT suggest for the three ticker loops or the createSession worker. ### SCOPE BOUNDARY Comment ONLY on: - `bigtable/internal/transport/session_pool.go` - `bigtable/internal/transport/session_pool_lifecycle.go` - `bigtable/internal/transport/session_pool_scaling.go` - `bigtable/internal/transport/session_pool_debug.go` - `bigtable/internal/transport/session_snapshot.go` - `bigtable/internal/transport/session_pool_*_test.go` - `bigtable/internal/transport/session_snapshot_test.go` Do NOT comment on additions to: - `session.go` / `session_vrpc.go` (WaitGoroutines / closeError additions — vetted) - `connpool.go` (`ChannelPickHintInto` helper — vetted) - `afe_picker.go` (`defaultAfeRandomSubsetSize` constant — vetted) - `debug_tracer.go` (3 new tags — vetted) These are supporting scaffolding, already reviewed by the 3 subagent reviewers in this stack. Only re-raise if something looks actively unsafe. ### EFFORT SCALING - ~4.9k LOC across 12 files. Do NOT paginate uniformly. - **First pass — the 4 hot source files, in this order:** 1. `session_pool.go` (559 LOC) 2. `session_pool_lifecycle.go` (538 LOC) 3. `session_pool_scaling.go` (311 LOC) 4. `session_pool_debug.go` (416 LOC) - **Second pass ONLY if a first-pass finding needs corroboration:** `session_snapshot.go` (594 LOC, mostly type defs), and the tests. Tests use `newTestPool`, which skips config wiring — do NOT flag bootstrap defaults on tests as if they were production paths. - If a first-pass finding is a real hazard from the list above, cite the file:line and the exact anchor pattern it violates. Do not file speculative "consider" comments.
1 parent 5d4c193 commit 683eda8

19 files changed

Lines changed: 5103 additions & 15 deletions

‎bigtable/internal/transport/afe_picker.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,12 @@ import (
1818
"math/rand/v2"
1919
)
2020

21+
// defaultAfeRandomSubsetSize is the default K for K-choice random draws
22+
// in LeastInFlightAfePicker / LeastLatencyAfePicker when the caller
23+
// doesn't specify one. Two candidates ("power of two choices") is the
24+
// standard K-choice draw size.
25+
const defaultAfeRandomSubsetSize = 2
26+
2127
// PickCandidate is one AFE the picker considered during a K-choice draw,
2228
// with the cost value the picker's decision rule used to score it.
2329
// Cost's interpretation depends on the picker in play: NumOutstanding

‎bigtable/internal/transport/connpool.go‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1066,3 +1066,22 @@ func (m multiError) Error() string {
10661066
}
10671067
return fmt.Sprintf("%s (and %d other errors)", s, n-1)
10681068
}
1069+
1070+
// channelPickHintKey identifies the *atomic.Int32 destination the caller
1071+
// wants BigtableChannelPool to publish the picked connEntry index into.
1072+
type channelPickHintKey struct{}
1073+
1074+
// ChannelPickHintInto returns a context that BigtableChannelPool will use to
1075+
// publish the picked connEntry index into the supplied *atomic.Int32. The
1076+
// caller can then read the value once the stream/invoke has returned.
1077+
//
1078+
// Used by Session creation to link sessions back to the channel they ride
1079+
// on — surfaced in the sessionz / channelz debug UIs. Untouched by callers
1080+
// that don't care: stampChannelPickHint short-circuits when the context
1081+
// lacks the key. Passing dst == nil returns ctx unchanged.
1082+
func ChannelPickHintInto(ctx context.Context, dst *atomic.Int32) context.Context {
1083+
if dst == nil {
1084+
return ctx
1085+
}
1086+
return context.WithValue(ctx, channelPickHintKey{}, dst)
1087+
}

‎bigtable/internal/transport/debug_tracer.go‎

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,32 @@ const (
116116
tagSessionPoolStuckSessionSwept = "session_pool_stuck_session_swept"
117117
tagSessionPoolDrainTimeout = "session_pool_drain_timeout"
118118
tagSessionPoolCreateFailed = "session_pool_create_failed"
119-
tagSessionPoolPickLostRace = "session_pool_pick_lost_race"
119+
// tagSessionPoolCreatePanic distinguishes a recovered panic inside
120+
// Tick's createSession fanout (streamFactory / NewSession / hook
121+
// wiring) from a plain error return (tagSessionPoolCreateFailed).
122+
// The two paths have very different root causes — a panic indicates
123+
// a client-side bug, an error is typically transient — so ops
124+
// should be able to grep them apart in the debug-tag counters.
125+
tagSessionPoolCreatePanic = "session_pool_create_panic"
126+
tagSessionPoolPickLostRace = "session_pool_pick_lost_race"
127+
tagSessionPoolConsecutiveFailuresTripped = "session_pool_consecutive_failures_tripped"
128+
// tagSessionPoolNoBudget fires when createSession's budget.Acquire
129+
// returns an error — either poolCtx cancel (teardown) or the
130+
// throttler's NewSessionCreationPenalty window expired without an
131+
// existing reservation being released. The count is the pool's
132+
// "opens throttled" signal; sustained emission means the budget
133+
// ceiling is too low for the offered load OR opens are hanging past
134+
// the penalty window.
135+
tagSessionPoolNoBudget = "session_pool_no_budget"
136+
137+
// tagSessionPoolCheckoutFailedCINil fires on SessionPoolImpl.Invoke's
138+
// early return when CheckoutSession failed — pool returns
139+
// InvokeResult{} with nil ClusterInfo, so stampAttempt downstream
140+
// records TagSessionAttemptNilClusterInfo without any session ever
141+
// being picked. Empirically dominates the nil-ClusterInfo population
142+
// during pool cold-start (waiters ctx.Done before first session
143+
// reaches Ready) and pool-close bursts (drainWaitersWithErr).
144+
tagSessionPoolCheckoutFailedCINil = "session_pool_checkout_failed_ci_nil"
120145

121146
// sessionList bookkeeping violations.
122147
//

‎bigtable/internal/transport/pool_sizer.go‎

Lines changed: 32 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ package internal
1717
import (
1818
"math"
1919
"sync"
20+
"sync/atomic"
2021

2122
spb "cloud.google.com/go/bigtable/apiv2/bigtablepb"
2223
)
@@ -53,10 +54,15 @@ type StatsFetcher func() *PoolStats
5354
// outside any peak-window cannot trigger a mass prune followed by a
5455
// cold-start scale-up on the next crest.
5556
type PoolSizer struct {
56-
mu sync.Mutex
57-
fetcher StatsFetcher
58-
minSessions int
59-
maxSessions int
57+
mu sync.Mutex
58+
fetcher StatsFetcher
59+
// minSessions / maxSessions are the authoritative pool-size bounds.
60+
// Stored as atomics so hot-path readers (CheckoutSession gate,
61+
// onClosing replace-check, createSession cap) can Load without
62+
// taking s.mu — pool.go used to duplicate these into its own
63+
// atomics; that duplication is now gone.
64+
minSessions atomic.Int32
65+
maxSessions atomic.Int32
6066
headroomPct float64 // Idle headroom as a fraction of sessions in use (e.g., 0.10 = 10%)
6167
newSessionQLen int // server-driven per-session pending queue length; divides PendingCount
6268
minIdleSessions int // floor on the idle cushion so headroom never collapses to 0
@@ -87,16 +93,26 @@ func NewPoolSizer(fetcher StatsFetcher, minSessions, maxSessions int, headroomPc
8793
if headroomPct <= 0 {
8894
headroomPct = float64(defaultPoolConfig().GetHeadroom())
8995
}
90-
return &PoolSizer{
96+
s := &PoolSizer{
9197
fetcher: fetcher,
92-
minSessions: minSessions,
93-
maxSessions: maxSessions,
9498
headroomPct: headroomPct,
9599
newSessionQLen: int(defaultPoolConfig().GetNewSessionQueueLength()),
96100
minIdleSessions: defaultMinIdleSessions,
97101
}
102+
s.minSessions.Store(int32(minSessions))
103+
s.maxSessions.Store(int32(maxSessions))
104+
return s
98105
}
99106

107+
// MinSessions returns the current pool-floor session count. Atomic load;
108+
// safe from any goroutine without taking the sizer lock. Callers that
109+
// need min/max/headroom as a consistent triple should use Decide instead.
110+
func (s *PoolSizer) MinSessions() int { return int(s.minSessions.Load()) }
111+
112+
// MaxSessions returns the current pool-ceiling session count. Atomic
113+
// load; same non-locking contract as MinSessions.
114+
func (s *PoolSizer) MaxSessions() int { return int(s.maxSessions.Load()) }
115+
100116
// UpdateConfig dynamically adjusts the sizer's capacity bounds,
101117
// headroom cushion, and per-session queue length at runtime. Called
102118
// from the server-config listener path; safe against concurrent
@@ -110,8 +126,12 @@ func (s *PoolSizer) UpdateConfig(config *spb.SessionClientConfiguration_SessionP
110126
s.mu.Lock()
111127
defer s.mu.Unlock()
112128

113-
s.minSessions = int(config.MinSessionCount)
114-
s.maxSessions = int(config.MaxSessionCount)
129+
// min/max are atomic so hot-path readers (pool gate + snapshot) see
130+
// the new bound the moment UpdateConfig returns; s.mu still brackets
131+
// the write with headroom/qlen so a Decide walking the sizer sees a
132+
// coherent config triple.
133+
s.minSessions.Store(config.MinSessionCount)
134+
s.maxSessions.Store(config.MaxSessionCount)
115135
// Mirror the constructor guard: a zero or negative headroom from
116136
// the server would render as HeadroomPct=0 on the loadz trace and
117137
// collapse IdleHeadroom to the MinIdleSessions floor — the pool
@@ -196,8 +216,8 @@ func (s *PoolSizer) Decide() ScaleDecision {
196216
defer s.mu.Unlock()
197217

198218
d := ScaleDecision{
199-
MinSessions: s.minSessions,
200-
MaxSessions: s.maxSessions,
219+
MinSessions: int(s.minSessions.Load()),
220+
MaxSessions: int(s.maxSessions.Load()),
201221
HeadroomPct: s.headroomPct,
202222
NewSessionQLen: s.newSessionQLen,
203223
MinIdleSessions: s.minIdleSessions,
@@ -233,7 +253,7 @@ func (s *PoolSizer) Decide() ScaleDecision {
233253
d.IdleHeadroom = s.minIdleSessions
234254
}
235255
d.DesiredRaw = d.SessionsInUse + d.IdleHeadroom
236-
d.DesiredCapacity = clamp(d.DesiredRaw, s.minSessions, s.maxSessions)
256+
d.DesiredCapacity = clamp(d.DesiredRaw, d.MinSessions, d.MaxSessions)
237257

238258
d.ImmediateCapacity = stats.ReadyCount
239259
d.EventualCapacity = stats.ReadyCount + stats.StartingCount

‎bigtable/internal/transport/session.go‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -195,6 +195,18 @@ type Session struct {
195195
// Embedded so bare field access (s.tracer, s.okRpcs, s.recordEvent,
196196
// ...) continues to compile once vRPC / lifecycle land.
197197
sessionDebug
198+
199+
// loops tracks readLoop + heartbeatLoop so a supervising owner
200+
// (SessionPoolImpl.Close) can wait for them to fully unwind — through
201+
// their notifyClosing / notifyClosed callback chains — before it
202+
// returns. Prevents readLoop's recordClose from racing package-level
203+
// metric var writes across test boundaries.
204+
loops sync.WaitGroup
205+
206+
// closeErr preserves the raw Recv error handed to handleClose. The
207+
// pool surfaces this on consecutive-failure breaker trips so operators
208+
// see the underlying server rejection instead of only the sentinel.
209+
closeErr atomic.Pointer[error]
198210
}
199211

200212
// SessionOption configures a Session at construction time.

‎bigtable/internal/transport/session_debug.go‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -405,3 +405,23 @@ func WithSessionLogger(logger *log.Logger) SessionOption {
405405
func WithSessionPoolName(name string) SessionOption {
406406
return func(s *Session) { s.tracer.setPoolName(name) }
407407
}
408+
409+
// setCloseErr records the raw error that ended the stream. First writer
410+
// wins so a follow-up close path (e.g. cancelActiveRPCs) can't overwrite
411+
// the original cause. Nil is treated as "no error" and ignored.
412+
func (s *Session) setCloseErr(err error) {
413+
if err == nil {
414+
return
415+
}
416+
s.closeErr.CompareAndSwap(nil, &err)
417+
}
418+
419+
// closeError returns the raw error that ended the stream, or nil if
420+
// none was recorded. Consulted by the pool to surface the underlying
421+
// server rejection on consecutive-failure trips.
422+
func (s *Session) closeError() error {
423+
if p := s.closeErr.Load(); p != nil {
424+
return *p
425+
}
426+
return nil
427+
}

‎bigtable/internal/transport/session_lifecycle.go‎

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -75,11 +75,23 @@ func (s *Session) Start(ctx context.Context, req *spb.OpenSessionRequest) error
7575
// is safe.
7676
s.hooks.onStart(ctx)
7777

78-
go s.readLoop(ctx)
79-
go s.heartbeatLoop(ctx)
78+
// Track readLoop + heartbeatLoop so WaitGoroutines can block until
79+
// their callback chains (notifyClosed → recordClose, etc.) have
80+
// unwound. Owners (SessionPoolImpl.Close) call WaitGoroutines during
81+
// teardown so no session-owned goroutine outlives the pool.
82+
s.loops.Add(2)
83+
go func() { defer s.loops.Done(); s.readLoop(ctx) }()
84+
go func() { defer s.loops.Done(); s.heartbeatLoop(ctx) }()
8085
return nil
8186
}
8287

88+
// WaitGoroutines blocks until readLoop and heartbeatLoop have fully
89+
// returned (including their notifyClosed / recordClose callback
90+
// chains). No-op if Start was never called.
91+
func (s *Session) WaitGoroutines() {
92+
s.loops.Wait()
93+
}
94+
8395
// ForceClose immediately transitions the session to StateClosed and cancels
8496
// every in-flight RPC. It is safe to call multiple times; only the first call
8597
// fires the tracer.recordClose and hooks.onClose callbacks.
@@ -450,6 +462,7 @@ func (s *Session) handleClose(err error) {
450462
s.notifyClosing()
451463
reason := streamEndReason(err)
452464
s.setCloseReason(reason)
465+
s.setCloseErr(err)
453466
// After setCloseReason (CompareAndSwap-once), the *final* reason may
454467
// be an earlier stamp (GoAway / MissedHeartbeat / Error) or the
455468
// streamEndReason we just computed. Only flag as abnormal when the

0 commit comments

Comments
 (0)