sushanb · 2026-07-27T18:41:17Z

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 (feat(bigtable): add per-AFE sessionList for the two-tier session pool #20224) — per-AFE bucketing data structure. Not yet merged.
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 feat(bigtable): add AFE picker (Simple / LeastInFlight / LeastLatency) #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

go build ./... passes.
go vet ./internal/transport/ clean.
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.

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

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.
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< 8000 /code> nil out elem under waitersMu (:329-358). Stats().PendingCount reads waitersCount.Load() — this is the sizer's queue-depth input.


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.
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.
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 feat(bigtable): add Session vRPC dispatch + slot lifecycle #20213 / feat(bigtable): add Session lifecycle (Start, Close, ForceClose, readLoop, heartBeatLoop) #20215 (state machine, one-in-flight, PeerInfo timing, retry oracle, heartbeat).
Per-AFE sessionList I1-I6 — shipped in feat(bigtable): add per-AFE sessionList for the two-tier session pool #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 feat(bigtable): add Session vRPC dispatch + slot lifecycle #20213 / feat(bigtable): add Session lifecycle (Start, Close, ForceClose, readLoop, heartBeatLoop) #20215 / feat(bigtable): add per-AFE sessionList for the two-tier session pool #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 (feat(bigtable): add Session vRPC dispatch + slot lifecycle #20213, feat(bigtable): add Session lifecycle (Start, Close, ForceClose, readLoop, heartBeatLoop) #20215, feat(bigtable): add per-AFE sessionList for the two-tier session pool #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:

session_pool.go (559 LOC)
session_pool_lifecycle.go (538 LOC)
session_pool_scaling.go (311 LOC)
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.



          
          
  
  
    
  
  
  
    
    Sorry, something went wrong.
  

  
    
  
    
      

              Uh oh!

              
There was an error while loading. Please reload this page.


  
  


                    
          
  
    
    
            gemini-code-assist[bot] reacted with eyes emoji
      
        
            
    All reactions
  


          
              
                👀
                  1 reaction

gemini-code-assist

Code Review

This pull request refactors the Bigtable session pool to implement AFE-aware (Application Front End) session routing and load balancing. It introduces a new 'sessionList' structure to group sessions by AFE and track PeakEWMA latencies per AFE, alongside a rewritten 'SessionPoolImpl' that uses a two-tier picking strategy. The changes also include extensive debug, observability, and lifecycle tracking features, such as consecutive-failure circuit breakers. The review feedback is highly constructive, suggesting an optimization in 'bumpCloseReason' to avoid unnecessary heap allocations by checking for key existence before calling 'LoadOrStore', and recommending renaming the variable 'AfeID' to avoid shadowing the type name of the same name.

gemini-code-assist · 2026-07-27T18:44:48Z

+	if label == "" {
+		label = "Unspecified"
+	}
+	c, _ := p.m.closesByReason.LoadOrStore(label, new(atomic.Int64))


To avoid unnecessary heap allocations of new(atomic.Int64) on every call to bumpCloseReason (even when the label already exists in the map), check if the key exists using Load first before calling LoadOrStore.

c, ok := p.m.closesByReason.Load(label) if !ok { c, _ = p.m.closesByReason.LoadOrStore(label, new(atomic.Int64)) } c.(*atomic.Int64).Add(1)

Applied — fc5c5d0. bumpCloseReason now Load-first, LoadOrStore only on miss. Same map, same key, same increment; the shared *atomic.Int64 on the hit path is no longer wrapped in a fresh alloc.

gemini-code-assist · 2026-07-27T18:44:48Z

+		AfeID, picked, decision := picker.PickAfe(ready)
+		p.recordPickDecision(decision, pickerName)
+		if picked {
+			if idle := p.sl.Checkout(AfeID); idle != nil {


The variable name AfeID shadows the type name AfeID defined in the same package. It is highly recommended to use a lowercase variable name like afeID or id to adhere to Go naming conventions and avoid shadowing types.

Suggested change

AfeID, picked, decision := picker.PickAfe(ready)

p.recordPickDecision(decision, pickerName)

if picked {

if idle := p.sl.Checkout(AfeID); idle != nil {

afeID, picked, decision := picker.PickAfe(ready)

p.recordPickDecision(decision, pickerName)

if picked {

if idle := p.sl.Checkout(afeID); idle != nil {

References

Avoid naming function parameters or variables identically to their type names to prevent shadowing the type name within the scope and to adhere to Go naming conventions.

Renamed the local var to afeID at session_pool.go:255,258 in fc5c5d0. Type AfeID stays exported; no other call sites touched.

Second of five PRs porting the session pool infrastructure from `feat/bigtable-sessionz-debug` (which powers the recycle-repro fleet). Stacks on googleapis#20224 (per-AFE sessionList). ## What lands **SessionPoolImpl** — the concrete two-tier read/write session pool for one resource. ~4000 LOC across five files: - `session_pool.go` (~550 LOC) — struct + constructor + Invoke + CheckoutSession (waiter queue, deadline propagation) + pluggable picker (Simple / LeastInFlight / LeastLatency) via the AFE picker from PR googleapis#20204. - `session_pool_lifecycle.go` (~530 LOC) — SessionHooks wiring (onStart/onActive/onClosing/onClose), consecutive-failure breaker, Close (5-phase teardown), and the WaitGoroutines / spawns.Wait choreography that guarantees no session-owned goroutine outlives the pool. - `session_pool_scaling.go` (~310 LOC) — 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` (~415 LOC) — 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` (~590 LOC) — the value-typed snapshot record the debug surface consumes; no live locks escape. - Five matching `_test.go` files (~2200 LOC): pool lifecycle, scaling, consecutive-failure breaker, AFE integration, debug surface, snapshot rendering, plus a K-choice bench. **Session helpers added** (pool-facing additions to files already touched by prior PRs, isolated to keep the diff readable): - `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** (minimal, isolated): - `afe_picker.go` — const `defaultAfeRandomSubsetSize = 2` (power-of-two-choices K-choice default; matches Java). - `debug_tracer.go` — three new tag constants: `tagSessionPoolCreatePanic` (distinguishes recovered panic from plain error return), `tagSessionPoolConsecutiveFailuresTripped` (breaker drain fired), `tagSessionPoolCheckoutFailedCINil` (Invoke returned InvokeResult{} with nil ClusterInfo — dominates the nil-ClusterInfo population during cold-start / pool-close bursts). - `connpool.go` — `ChannelPickHintInto(ctx, *atomic.Int32)` context helper used by createSession to link each session to its channel (surfaced in sessionz / channelz in a later PR). No-op when the channel pool doesn't consume the hint. ## What does NOT land yet - The `SessionPool` / `Invoker` interfaces (follow-up PR alongside sessionClient / sessionTable — those consumers land in PR-3+). - The `SessionPoolImpl` factory / `NewSessionPool` wiring against `bigtable.Client` (PR-3). - Debug pages (sessionz / afez / flightz / loadz — landing under `bigtable/debugview/` in a 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)

- session_pool.go: rename local var `AfeID` → `afeID` in CheckoutSession to avoid shadowing the exported type `AfeID`. - session_pool_lifecycle.go: `bumpCloseReason` — Load first, LoadOrStore only on miss. Avoids allocating `new(atomic.Int64)` on every hit-path call (close-reason bumps are frequent).

…ClientConfig Fallback values (headroom, LB strategy, budget size, penalty, consecutive- failure threshold) used to be literals hardcoded in NewSessionPoolImpl. Read them from defaultClientConfig.SessionConfiguration.SessionPoolConfiguration (via the existing defaultPoolConfig helper) so the fallback table has a single source of truth in default_client_config.go. Behavior is bootstrap-only — every real caller registers with ClientConfigurationManager, which fires UpdateConfig synchronously and replaces these with server-driven values before the pool serves traffic.

…ClientConfig Fallback values (headroom, LB strategy, budget size, penalty, consecutive- failure threshold) in NewSessionPoolImpl now read from defaultClientConfig.SessionConfiguration.SessionPoolConfiguration via the existing defaultPoolConfig helper — single source of truth in default_client_config.go instead of literals duplicated in the constructor. Also update SESSION_POOL_SPEC.md #5 to drop the stale "default 10%" parenthetical (default now sourced from the proto). Ports the same change opened on PR googleapis#20225 against upstream (commit landed there via the pool-bundle PR).

Column alignment on the fakeVRpcDesc method definitions. Fixes the vet.sh goimports check that was failing on CI.

sushanb · 2026-07-27T19:15:01Z

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.

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

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.
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.
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.
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.
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 feat(bigtable): add Session vRPC dispatch + slot lifecycle #20213 / feat(bigtable): add Session lifecycle (Start, Close, ForceClose, readLoop, heartBeatLoop) #20215 (state machine, one-in-flight, PeerInfo timing, retry oracle, heartbeat).
Per-AFE sessionList I1-I6 — shipped in feat(bigtable): add per-AFE sessionList for the two-tier session pool #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 feat(bigtable): add Session vRPC dispatch + slot lifecycle #20213 / feat(bigtable): add Session lifecycle (Start, Close, ForceClose, readLoop, heartBeatLoop) #20215 / feat(bigtable): add per-AFE sessionList for the two-tier session pool #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 (feat(bigtable): add Session vRPC dispatch + slot lifecycle #20213, feat(bigtable): add Session lifecycle (Start, Close, ForceClose, readLoop, heartBeatLoop) #20215, feat(bigtable): add per-AFE sessionList for the two-tier session pool #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.

mutianf · 2026-07-27T19:59:53Z

+	// Writes go under p.mu so PoolSnapshot reads a consistent pair;
+	// hot-path readers Load() without the lock (no cross-field invariant
+	// with picker/budget/threshold).
+	minSessions        atomic.Int32


Do we need them on the SessionPool struct?

Dropped in d84ddd6 — removed the duplicate atomic.Int32 minSessions/maxSessions fields from SessionPoolImpl. PoolSizer already owned them (behind s.mu); it now stores them as atomic.Int32 with MinSessions()/MaxSessions() accessors so hot-path readers (CheckoutSession slow-path gate, onClosing replace gate, createSession cap check, PoolSnapshot) go through the sizer without any mutex. UpdateConfig no longer needs to write them under p.mu; the picker swap is the only field left in that critical section.

mutianf · 2026-07-27T20:02:23Z

+	// server-driven values before the pool serves traffic.
+	defaultCfg := defaultPoolConfig()
+	fetcher := func() *PoolStats { return pool.Stats() }
+	pool.sizer = NewPoolSizer(fetcher, min, max, float64(defaultCfg.GetHeadroom()))


I think it needs to be defaultCfg.getMinSessions() and defaultCfg.getMaxSessions?

Applied in d84ddd6. NewSessionPoolImpl now falls back to defaultCfg.GetMinSessionCount() / GetMaxSessionCount() when min <= 0 / max <= 0 before constructing the sizer — otherwise a caller passing zero bounds would see the sizer clamp DesiredCapacity to 0 and never open a session before UpdateConfig arrives from ClientConfigurationManager.

mutianf · 2026-07-27T20:14:13Z

+			recordDebugTag(tagSessionPoolPickLostRace)
+		}
+
+		// Slow path: picker returned nil. Dying sessions leave sl.readyCount


Suggested change

// Slow path: picker returned nil. Dying sessions leave sl.readyCount

// Slow path: picker returned nil or 2 checkoutSession raced and returned the same AFE id. Dying sessions leave sl.readyCount

Accepted verbatim in d84ddd6.

mutianf · 2026-07-27T20:30:58Z

+func (p *SessionPoolImpl) drainWaitersWithErr(err error) int {
+	p.waitersMu.Lock()
+	defer p.waitersMu.Unlock()
+	n := 0


Renamed to woken in d84ddd6.

mutianf · 2026-07-27T20:31:55Z

+
+	ready := 0
+	inUse := 0
+	for _, sh := range p.sl.AllHandles() {


sl.AllHandles is taking a snapshot of the session list, do we need to run the loop within the lock?

Fixed in d84ddd6. The sl.AllHandles walk now runs OUTSIDE p.mu — AllHandles takes its own sl.mu snapshot, and per-session State() / outstanding.Load() are all atomics. p.mu now only brackets the startingSessions + pendingStarts read (still plain non-atomic fields). Documented on Stats() that starting → ready transitions between the two snapshots can transiently under-count by one — Sizer.Decide self-corrects on the next Tick, which is the sole consumer.

mutianf · 2026-07-27T20:58:16Z

+	woken := p.drainWaitersWithErr(tripErr)
+	if woken > 0 {
+		recordDebugTag(tagSessionPoolConsecutiveFailuresTripped)
+	}


add a TODO: if there are consecutive unimplemented failures we should fallback to unary

Added the TODO in d84ddd6 on the drainWaitersWithErr success arm. Kept a note that the routing flip is SessionClient / Diverter's decision, not the pool's — the pool only surfaces the trip cause; the layer above owns the choice to divert future opens to the classic (unary) path.

mutianf · 2026-07-27T21:01:59Z

+	// pendingStarts counts createSession goroutines that haven't yet
+	// reached streamFactory success. Prevents back-to-back Ticks in
+	// the streamFactory window from re-requesting the same delta.
+	pendingStarts int


does this belong to the pool or PoolSizer?

Kept on the pool in d84ddd6. pendingStarts is transferred atomically with the startingSessions map insert under p.mu (createSession session_pool_scaling.go:253-256: single lock does p.pendingStarts-- + p.startingSessions[sh] = struct{}{}), so hoisting to PoolSizer would either force the sizer to know about SessionHandles or split the mutation across two locks and race the map insert. Left the reservation counter on the pool where it's paired with startingSessions.

mutianf · 2026-07-27T21:03:06Z

+	}()
+
+	if err := p.budget.Acquire(dialCtx); err != nil {
+		return fmt.Errorf("failed to acquire session creation budget: %w", err)


log a debugTag error session_pool_no_budget

Added in d84ddd6. New tagSessionPoolNoBudget const in debug_tracer.go and recordDebugTag(tagSessionPoolNoBudget) fires on the budget.Acquire failure path before the error is returned. Kept distinct from tagSessionPoolCreateFailed so ops can grep the throttled path (budget ceiling exhausted / poolCtx cancel / penalty window elapsed) apart from stream-open errors.

mutianf · 2026-07-27T21:07:58Z

+	defer func() {
+		// Success path releases early so budget isn't held for the
+		// Session's lifetime; this fallback covers failure paths.
+		if !budgetReleased {


Hmm I think when it enters this function it's always session creation failure? Otherwise it'll be held for the session's lifetime.. do we need the sucess local variable?

Right, dropped in d84ddd6. The success path calls p.budget.Release(true) explicitly and sets budgetReleased=true, so the deferred fallback only ever fires on failure paths (streamFactory error, cap-gate hit, or Session.Start error). The defer just calls Release(false) directly now — no need for the success local. Also added a comment explaining the invariant so a future edit doesn't reintroduce a Release(true) fallback.

mutianf · 2026-07-27T21:15:33Z

+		}
+	}()
+
+	if err := p.budget.Acquire(dialCtx); err != nil {


budget.Acquire blocks until budget frees up. Is this gonna be a problem? https://github.com/googleapis/google-cloud-go/blob/main/bigtable/internal/transport/session_throttler.go#L77

Not a problem in practice — no code change in d84ddd6, but here's the shape:

Acquire runs outside p.mu. createSession does budget.Acquire on a plain dialCtx (no pool lock held), so a stalled Acquire never blocks concurrent CheckoutSession or Tick.

Wait is bounded. adaptiveSessionThrottler.Acquire (session_throttler.go:77) selects on time.After(t.penalty); penalty defaults to NewSessionCreationPenalty (60 s in defaultClientConfig). Server can drop it at runtime via UpdateConfig.

Timeout releases the reservation. On timeout / poolCtx cancel, Acquire returns an error, createSession's reserved defer decrements pendingStarts, and the goroutine exits without spawning. Also emits session_pool_no_budget now (per the sibling comment) so ops can see the throttle rate.

So worst-case is 60s of a createSession goroutine parked in Acquire, which p.spawns.Wait picks up during Close. No accumulation, no hot-path stall.

Thirteen inline comments from the human review round on PR googleapis#20225. State-based abnormal-close classification and consolidating min/max onto PoolSizer are the two behaviour-touching pieces; the rest tighten lock scope, add debug tags, and clarify comments. - #3660876483 (session_pool.go:120): pendingStarts left on the pool — it's transferred atomically with the startingSessions map insert under p.mu, so hoisting to PoolSizer would either need SessionHandle visibility on the sizer or a separate atomic pair that races the map mutation. Reply-only. - #3660499690 (session_pool.go:131): dropped duplicate minSessions/maxSessions atomic.Int32 from SessionPoolImpl; the same bounds already live on PoolSizer. Added MinSessions()/MaxSessions() atomic accessors on *PoolSizer. All hot-path readers (CheckoutSession slow-path gate, onClosing replace gate, createSession cap check, PoolSnapshot) now go through the sizer. - #3660515434 (session_pool.go:220): min <= 0 / max <= 0 now fall back to defaultCfg.GetMinSessionCount()/GetMaxSessionCount() before the sizer is constructed. Prevents the sizer clamping DesiredCapacity to 0 on callers that instantiate the pool with zero bounds. - #3660587553 (session_pool.go:275): accepted the suggested Slow-path comment text verbatim. - #3660687707 (session_pool.go:345): renamed 'n' → 'woken' in drainWaitersWithErr. - #3660693686 (session_pool.go:367): Stats() now runs the sl.AllHandles walk outside p.mu — snapshot is taken by sl under sl.mu and per-session State()/outstanding reads are atomic. p.mu now only brackets the startingSessions + pendingStarts read. - #3660711591 (session_pool.go:415): sizer.UpdateConfig, budget.UpdateConfig, and consecutiveFailureThreshold.Store already ran outside p.mu; kept them there. Narrowed the in-lock section to just the picker swap and added a comment explaining why sizer/budget updates don't need p.mu. - #3660747369 (session_pool.go:479): added a doc comment naming the poolWait → client_blocking_latencies mapping (Java internal name: throttling latencies) so the OTel wiring PR has a hook. - #3660848996 (session_pool_lifecycle.go:364): refactored noteAbnormalCloseIfAny to state-based classification via sh.activated.Load() — signature changed from *Session to *SessionHandle. A session that never reached StateReady (onActive never fired) is the exact "open failed" signal; sessions that activated and later died (server GoAway, missed heartbeat, stream errors) no longer trip the breaker regardless of reason string. Removes the reason-whitelist drift risk when the server invents a new close reason. Test helpers gained injectStartingSession and abnormalOnCloseFor's abnormal path routes through it; the trip- cause test uses starting handles too. - #3660854904 (session_pool_lifecycle.go:391): added TODO for the Unimplemented → unary fallback (routing flip belongs to SessionClient / Diverter, not the pool). - #3660882862 (session_pool_scaling.go:182): added recordDebugTag(tagSessionPoolNoBudget) on budget.Acquire failure and the new const to debug_tracer.go's catalog. - #3660911070 (session_pool_scaling.go:190): dropped the 'success' local — the deferred fallback only fires on failure paths (success path releases the budget explicitly and flips budgetReleased), so the defer can just Release(false) directly. - #3660954731 (session_pool_scaling.go:181): reply-only. budget.Acquire blocks outside p.mu so a stalled Acquire doesn't back up CheckoutSession; the wait is bounded by NewSessionCreationPenalty (60s default); on timeout the reserved defer releases pendingStarts and the goroutine exits without spawning.

sushanb · 2026-07-27T23:10:59Z

sushanb has read and replied to all comments.

mutianf · 2026-07-28T01:01:13Z

+	// budgetReleased is the sole gate: the success path calls
+	// budget.Release(true) explicitly and sets budgetReleased=true, so
+	// any deferred fallback only fires on failure — no need for a
+	// separate `success` local. Passing false is correct because the


nit:

remove "- no need for a separate success local."

Removed in d54717c. Also cherry-picked to sessionz as d7701be.

…c-comment fragment Follow-up to mutianf review nit on PR googleapis#20225 — the fragment referenced a local that no longer exists (dropped in d84ddd6). The surrounding budgetReleased-gate rationale stays.

…c-comment fragment Follow-up to mutianf review nit on PR googleapis#20225 — the fragment referenced a local that no longer exists (dropped in the earlier mutianf-nits commit). The surrounding budgetReleased-gate rationale stays.

Third of five PRs porting the session pool infrastructure from feat/bigtable-sessionz-debug. Stacks on googleapis#20225 (SessionPoolImpl). - internal/session/api.go — public interfaces (ChannelPool, Config, SessionClient, SessionTableAPI, DebugAccess) - internal/session/client.go — SessionClient impl: dedicated channel pool (no primer), ClientConfigurationManager wiring, OpenSessionTable / OpenAuthorizedView / OpenMaterializedView factories - internal/session/table.go — SessionTable impl with lazy read/write pools, per-attempt metrics stamping - internal/session/lazy_pool.go — Invoker + SessionPool interfaces, open-on-first-use lazy pool wrapper - internal/session/debug.go — DebugAccess impl surfacing pool snapshots Transport package additions to support the above: - transport/debug_api.go — SessionDebugProvider / ChannelDebugProvider / ConfigDebugProvider interfaces + ChannelPoolDebug / SessionRef DTOs (lives in transport, not bigtable, so both bigtable.Client and internal/session.SessionClient can implement without an import cycle) - transport/diverter.go — sessionPicks/classicPicks counters + DiverterSnapshot + Snapshot() method - transport/connpool.go — ChannelSnapshot + ChannelPoolSnapshot type + ChannelPoolSnapshot() method + WithInstanceName / WithAppProfile options for channelz labelling - transport/debug_tracer.go — exported DebugTag type + RecordDebugTag + TagSessionAttemptNilClusterInfo / TagSessionAttemptEmptyClusterID catalog constants - transport/session_descriptors.go — SessionType.ProtoName() for human-readable pool identifiers - transport/direct_access_checker.go — renames newPingAndWarmDirectAccessChecker to NewPingAndWarmDirectAccessChecker so the session package can construct one across the package boundary; nil-guards the primer.Prime call so session-based clients (which warm channels on-demand via OpenSession, not eagerly at pool-init) can pass a nil primer sessionClient.Close() snapshots owned resources under poolsMu and releases the lock before running Close/Shutdown/Cancel calls so a snapshot method holding poolsMu never deadlocks teardown. Post-Close Opens surface a distinct ErrSessionClientClosed sentinel instead of misleading errReadPoolNil / ErrWriteNotSupported.

## Summary Third of five PRs porting the session-pool infrastructure from `feat/bigtable-sessionz-debug` into upstream. Introduces `internal/session/` — a proto-native SessionClient + SessionTable API sitting on top of PR-2's SessionPoolImpl. - `internal/session/api.go` — public interfaces (`ChannelPool`, `Config`, `SessionClient`, `SessionTableAPI`, `DebugAccess`). - `internal/session/client.go` — `SessionClient` impl: dedicated channel pool (no primer), `ClientConfigurationManager` wiring, `OpenSessionTable` / `OpenAuthorizedView` / `OpenMaterializedView` factories that mint lazily-opened per-resource pools keyed by `{resource, permission}`. - `internal/session/table.go` — `SessionTable` impl. Two `*lazyPool` (read + write); MV is read-only (write pool nil, `MutateRow` returns `ErrWriteNotSupported`). `stampAttempt` sources per-attempt `cluster_id` / `zone_id` / peer fields from typed `InvokeResult.ClusterInfo` and `InvokeResult.PeerInfo` per CLIENT_SIDE_METRICS_SPEC #1. - `internal/session/lazy_pool.go` — `Invoker` + `SessionPool` interfaces + open-on-first-use lazy wrapper. Failed opens are NOT cached; the next call retries. - `internal/session/debug.go` — `DebugAccess` impl surfacing pool snapshots for sessionz/loadz/channelz/configz. ### Transport additions to support the above - `transport/debug_api.go` (new) — `SessionDebugProvider` / `ChannelDebugProvider` / `ConfigDebugProvider` interfaces + `ChannelPoolDebug` / `SessionRef` DTOs. Lives in transport (not bigtable) so `bigtable.Client` and `internal/session.SessionClient` can implement without an import cycle. - `transport/diverter.go` — `sessionPicks` / `classicPicks` counters, `DiverterSnapshot`, `Snapshot()`. - `transport/connpool.go` — `ChannelSnapshot` + `ChannelPoolSnapshot` type + method, `WithInstanceName` / `WithAppProfile` options for channelz labelling. - `transport/debug_tracer.go` — exported `DebugTag` + `RecordDebugTag` + `TagSessionAttemptNilClusterInfo` / `TagSessionAttemptEmptyClusterID` catalog constants. - `transport/session_descriptors.go` — `SessionType.ProtoName()` for human-readable pool identifiers. - `transport/direct_access_checker.go` — renames `newPingAndWarmDirectAccessChecker` → `NewPingAndWarmDirectAccessChecker` (constructor exported) and nil-guards `primer.Prime` so session-based clients can pass a nil primer. Session clients warm channels on-demand via `OpenSession`, not eagerly at pool-init. ### Lifecycle correctness `sessionClient.Close()` snapshots owned resources under `poolsMu` and releases the lock before running `Close` / `Shutdown` / `Cancel` calls, so a snapshot method holding `poolsMu` never deadlocks teardown. Post-Close Opens surface a distinct `ErrSessionClientClosed` sentinel rather than misleading `errReadPoolNil` / `ErrWriteNotSupported`. ## Stack - **PR-1** #20224 (sessionList) — merged - **PR-2** #20225 (SessionPoolImpl) — open; this PR stacks on it - **PR-4** (this PR) - PR-5 will follow with the bigtable-package integration + debugview. The diff on this PR includes PR-2's commits until #20225 merges into main. ## Test plan - [x] `go build ./internal/session/ ./internal/transport/ ./...` - [x] `go test ./internal/session/ -race -count=1 -short -timeout=180s` - [x] `go test ./internal/transport/ -race -count=1 -short -skip 'AfeLbSim|TestHighQpsSession' -timeout=240s` - [x] `gofmt -l ./internal/session/ ./internal/transport/` — clean - [x] `go vet ./internal/session/ ./internal/transport/` — clean - [x] Reviewed against the 4 behavioral specs (SESSION_SPEC, SESSION_CLIENT_SPEC, SESSION_POOL_SPEC, CLIENT_SIDE_METRICS_SPEC) and SESSION_COMPONENT_SPEC (boundary rules) — PASS.

🤖 I have created a release *beep* *boop* --- ## [1.52.0](bigtable/v1.51.0...bigtable/v1.52.0) (2026-08-03) ### Features * **bigtable:** Add AFE picker (Simple / LeastInFlight / LeastLatency) ([#20204](#20204)) ([bcbf714](bcbf714)) * **bigtable:** Add ClientConfig.DisableSession to opt out of session backend ([#20297](#20297)) ([7ee5e44](7ee5e44)) * **bigtable:** Add getClientConfigDirectAccessChecker for session pools ([#20209](#20209)) ([3b8d30a](3b8d30a)) * **bigtable:** Add NoOpChannelPrimer for session channel pools ([#20208](#20208)) ([d055a8a](d055a8a)) * **bigtable:** Add per-AFE sessionList for the two-tier session pool ([#20224](#20224)) ([dbf0c3f](dbf0c3f)) * **bigtable:** Add protoRowToRow conversion helper for TableShim ([#20257](#20257)) ([1297143](1297143)) * **bigtable:** Add Session debug surface (observability fields + methods) ([#20211](#20211)) ([d8d3e16](d8d3e16)) * **bigtable:** Add Session lifecycle (Start, Close, ForceClose, readLoop, heartBeatLoop) ([#20215](#20215)) ([b9e53c6](b9e53c6)) * **bigtable:** Add Session struct + state machine ([#20117](#20117)) ([09acbb3](09acbb3)) * **bigtable:** Add session.Config.EnableDebug to gate sessionz debug state ([#20247](#20247)) ([ce74c31](ce74c31)) * **bigtable:** Add SessionClient + SessionTable + lazyPool ([#20228](#20228)) ([ab2c96c](ab2c96c)) * **bigtable:** Add SessionPoolImpl (two-tier pool + scaling + debug) ([#20225](#20225)) ([683eda8](683eda8)) * **bigtable:** Rename session pool display to <resource-id>-<PERM> ([#20248](#20248)) ([35e146e](35e146e)) * **bigtable:** Route Client.Open()-returned *Table through the Diverter ([#20273](#20273)) ([2b81c7d](2b81c7d)) * **bigtable:** State-based classification for abnormal session close ([#20243](#20243)) ([f2905b7](f2905b7)) * **bigtable:** TableShim fallback to classic on session UNIMPLEMENTED ([#20269](#20269)) ([36540af](36540af)) * **bigtable:** TTL-on-idle cache for per-resource session.TableAPI ([#20263](#20263)) ([00b2a49](00b2a49)) * **bigtable:** Wire Diverter on Client and route Open* via TableShim ([#20256](#20256)) ([b32fbd7](b32fbd7)) ### Bug Fixes * **bigtable:** AFE picker latency signal — subtract poolWait and compute TransportLatency = wire − backend at source ([#20281](#20281)) ([bb8c4d5](bb8c4d5)) * **bigtable:** Guard NewStream OnFinish against grpc-go double-fire ([#20295](#20295)) ([b51da29](b51da29)) * **bigtable:** Real per-resource pool teardown on sessionTable.Close + cache close-race gate ([#20264](#20264)) ([599aea9](599aea9)) * **bigtable:** Session.durations / session.uptime — set explicit histogram bucket boundaries ([#20276](#20276)) ([97eee22](97eee22)) * **bigtable:** SessionTableHandle self-heals across cache eviction ([#20296](#20296)) ([0dd98cd](0dd98cd)) * **bigtable:** Translate ctx errors to gRPC status on session vRPC ([#20299](#20299)) ([0f3b2a5](0f3b2a5)) * **bigtable:** Treat PingAndWarm NotFound as a successful prime ([#20219](#20219)) ([a1557ad](a1557ad)) ### Performance Improvements * **bigtable:** Delete periodic Tick loop; sizing is event-driven ([#20285](#20285)) ([2c096bd](2c096bd)) * **bigtable:** Drop pick_lost_race debug tag from CheckoutSession hot path ([#20280](#20280)) ([bd0e400](bd0e400)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>

sushanb requested review from a team as code owners July 27, 2026 18:41

product-auto-label Bot added the api: bigtable Issues related to the Bigtable API. label Jul 27, 2026

sushanb force-pushed the bigtable-session-pool branch from 6bd7304 to cd7c01c Compare July 27, 2026 18:44

gemini-code-assist Bot reviewed Jul 27, 2026

View reviewed changes

sushanb force-pushed the bigtable-session-pool branch from cd7c01c to 229646c Compare July 27, 2026 18:45

sushanb added 2 commits July 27, 2026 18:50

fix(bigtable): goimports formatting on session_pool_debug_test.go

bb9f924

Column alignment on the fakeVRpcDesc method definitions. Fixes the vet.sh goimports check that was failing on CI.

mutianf reviewed Jul 27, 2026

View reviewed changes

sushanb mentioned this pull request Jul 27, 2026

feat(bigtable): add SessionClient + SessionTable + lazyPool #20228

Merged

6 tasks

mutianf approved these changes Jul 28, 2026

View reviewed changes

fix(bigtable): drop stale "no need for a separate success local" do…

d54717c

…c-comment fragment Follow-up to mutianf review nit on PR googleapis#20225 — the fragment referenced a local that no longer exists (dropped in d84ddd6). The surrounding budgetReleased-gate rationale stays.

sushanb merged commit 683eda8 into googleapis:main Jul 28, 2026
19 checks passed

release-please Bot mentioned this pull request Jul 27, 2026

chore(main): release bigtable 1.52.0 #20207

Merged

	// Slow path: picker returned nil. Dying sessions leave sl.readyCount
	// Slow path: picker returned nil or 2 checkoutSession raced and returned the same AFE id. Dying sessions leave sl.readyCount

Uh oh!

Conversation

Uh oh!

Summary

Stack

What lands

What does NOT land yet

Test plan

Reviewer guide

Guide 1 — mutianf (human)

What this PR does

Recommended read order

Flow of events

Key invariants

What NOT to worry about

Danger zones

Guide 2 — mutianf-bot (automated reviewer)

REAL HAZARDS to flag

FALSE-POSITIVE SUPPRESSION

SCOPE BOUNDARY

EFFORT SCALING

Uh oh!

Choose a reason for hiding this comment

Code Review

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Guide 1 — mutianf (human)

What this PR does

Recommended read order

Flow of events

Key invariants

What NOT to worry about

Danger zones

Guide 2 — mutianf-bot (automated reviewer)

REAL HAZARDS to flag

FALSE-POSITIVE SUPPRESSION

SCOPE BOUNDARY

EFFORT SCALING

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!