feat(bigtable): TTL-on-idle cache for per-resource session.TableAPI - #20263
Merged
sushanb merged 30 commits intoJul 30, 2026
Conversation
Three coordinated changes so a follow-up session-data-path patch can flip traffic through the shim without further Client/OpenTable churn: 1. Client owns a *transport.Diverter, constructed with sessionLoad=0.0 in NewClientWithConfig. Every call stays on the classic path until a future change wires in the session backend and bumps the ratio. 2. Open / OpenTable / OpenAuthorizedView / OpenMaterializedView move out of client.go into a new open.go. OpenTable & friends now return NewTableShim(classic, nil, c.diverter) — the shim wraps the same classic tableImpl as before with routing added on top. Open() (returning *Table) stays classic-only for callers that need the concrete pointer type. 3. TableShim gains a pickSession() gate: session TableAPI is optional; with session==nil pickSession short-circuits before consulting the diverter so the pick-count histogram doesn't record session picks that got silently downgraded here. ReadRow + Apply route through pickSession() instead of calling diverter.UseSession() directly. Also: DefaultDynamicChannelPoolConfig.MinConns 10 -> 4. Session client uses a pool of 4 as its footprint default, and today ValidateDynamicConfig rejects that with "initial connPoolSize (4) must be between MinConns (10) and MaxConns (200)". Lowering the floor to 4 lets the session pool validate under the same default config the classic pool uses. No behavior change on the classic path: sessionLoad=0.0 makes UseSession() return false in every case, and the pickSession() nil-guard would defend even if it didn't. All existing bigtable tests pass.
Follow-up on the diverter wiring in this PR. Makes Client.Open route
through the same TableShim path that OpenTable / OpenAuthorizedView /
OpenMaterializedView already use, without changing Open's *Table
return type — so classic callers that hold a concrete *Table pointer
(BulkMutation and any external code) will get session routing when the
session backend lands in a follow-up PR.
Wiring:
- Table gains a divertible TableAPI field. Nil for classic-only clients.
- Table.Apply / Table.ReadRow become one-line gates:
if t.divertible != nil { return t.divertible.X(...) }
return t.xClassic(...)
- Table.applyClassic / Table.readRowClassic hold the original bodies
verbatim — no metrics / tracer / retry behavior change on classic.
- tableImpl grows an explicit ReadRow override; both tableImpl.Apply
and tableImpl.ReadRow now call the *Classic helpers directly,
bypassing the gate. This breaks the recursion loop: the shim's
classic branch dispatches into tableImpl, which lands on applyClassic
/ readRowClassic instead of coming back through Table.Apply /
Table.ReadRow.
- Open populates t.divertible via a new buildDivertible helper that
constructs NewTableShim(&tableImpl{Table: <snapshot with divertible
zeroed>}, nil, c.diverter) when c.diverter != nil. The
snapshot-with-divertible-zeroed is defense in depth against a future
reader who removes the tableImpl overrides.
- OpenTable / OpenAuthorizedView / OpenMaterializedView refactored to
reuse buildDivertible so all four Open* methods construct the shim
identically. Session TableAPI is nil today; the follow-up wires it.
Behavioral guarantees:
- Conditional Apply (CheckAndMutateRow) still routes to classic —
TableShim.Apply enforces that on the shim side; the Table-level gate
is unconditional so every conditional passes through the shim, which
re-routes it to classic. Same end state.
- ReadRows / SampleRowKeys / ApplyBulk / ApplyReadModifyWrite are
untouched — no session equivalent, no gate needed.
- Classic-only clients (c.diverter == nil) short-circuit at
buildDivertible; Table.divertible stays nil; both gates fall through
to *Classic on the fast path. Zero perf change.
All target tests green.
gemini review on PR googleapis#20256 flagged that pickSession would NPE if a caller built a TableShim without a diverter (TableShim is exported, so this is reachable from tests and external code even though in-repo callers always wire one via NewTableShim). Widens the short-circuit to `t.session == nil || t.diverter == nil`. Nil-session guard still runs FIRST — same reason as before: consulting the diverter is side-effectful (per-outcome pick counters), and we don't want session picks that got downgraded to classic here inflating the histogram.
…ableShim" This reverts commit 5b48ef4.
Folds PR googleapis#20258 into this PR. TableShim's session backend changes from the classic-shaped TableAPI (row string + bigtable.Row) to the internal/session.TableAPI shape (proto-native *btpb.SessionReadRow{Request,Response} + *btpb.SessionMutateRow {Request,Response}). TableShim owns the proto ↔ public-types translation so the session package can stay proto-native. - ReadRow: parses ReadOptions via classic makeReadSettings/readSettings so every existing option (RowFilter, WithFullReadStats, etc.) works on the session path; then translates (row, filter) → SessionReadRowRequest, calls session.ReadRow, feeds resp.Stats through WithFullReadStats, and converts resp.Row via protoRowToRow (added in googleapis#20257, now on main). - Apply: nil mutations and conditional mutations (CheckAndMutateRow) route to classic — CheckAndMutateRow has no session vRPC equivalent, and nil-m to classic surfaces the classic client's existing nil-handling error rather than panicking on m.ops here. - useSession() helper is nil-safe on both session and diverter (replaces the earlier pickSession); callers can wire a TableShim with nil session and every routing decision falls through to classic. Signature change (internal-only): NewTableShim's session parameter type is now session.TableAPI (an internal package). In-repo callers pass nil for session today; nil is the zero value of any interface, so the change is source-compatible for those. Tests: replaces the mockTableAPI-as-session pattern with a proto-native mockSessionTable. Covers routing, nil-session fallback, session error propagation, conditional-always-classic, and the already-existing TestProtoRowToRow contract. Closes PR googleapis#20258 (the standalone version); folded here so PR googleapis#20256's review scope is one coherent unit — Diverter wiring + TableShim proto-native shape together.
goimports flagged bigtable/client.go on CI. The Option B revert left a double blank line where the four Open* methods used to sit; goimports canonicalizes that to a single blank line. Pure whitespace fix.
Adds bigtable/open_test.go with 5 tests pinning the factory contract introduced by this PR. Focused on the composition (which shape each factory returns + how it wires the client's Diverter) rather than re-testing routing behavior — TableShim's own tests already cover nil-session-routes-to-classic (TestTableShim_NilSession_AllMethodsFallBackToClassic on main). Tests: - TestOpen_ReturnsBareTable — Open returns plain *Table (no session-routing wrapper). Callers holding *Table stay on the classic path regardless of Client's diverter setting. - TestOpenTable_ProducesNilSessionShim — OpenTable returns *TableShim with session == nil and classic == *tableImpl. Uses SessionLoad=1.0 on the diverter to prove useSession() still returns false because session is nil (the nil-session short-circuit runs before consulting the diverter). - TestOpenAuthorizedView_ProducesNilSessionShim — same, plus authorizedView threaded through to the inner Table. - TestOpenMaterializedView_ProducesNilSessionShim — same, plus materializedView threaded through (table empty since MVs are addressed by view name only). - TestOpenFactories_ShareOneClientDiverter — every Open* factory references the SAME *Diverter, and mutating it via SetSessionLoad is visible from every shim (proves the reference is live, not a copy). This is the invariant a future ConfigurationManager.SetSessionLoad depends on to update every open resource at once. Test bootstrap uses a hand-built minimal *Client (project + appProfile + featureFlagsMD + diverter) rather than NewClientWithConfig to keep the tests dial-free and hermetic — the factory code paths only read those fields.
Follow-up to googleapis#20256 (Diverter wiring). Makes Open* actually produce session-routed TableShims when the caller opts in via ClientConfig.EnableSessionPool = true. Without the opt-in the shim's session TableAPI stays nil and every routing decision falls through to classic (existing behavior; existing tests still pass). Wiring: - Client gains three fields: sessionImpl session.Client sessionTablesMu sync.Mutex sessionTables map[string]session.TableAPI // per-resource cache - ClientConfig gains EnableSessionPool bool. When true, NewClientWithConfig calls session.NewClient(ctx, project, instance, appProfile, metricsProvider, opts...) with the same option set as the classic pool (endpoint/credentials/dial hooks match), then wires sc.AddSessionLoadListener(c.diverter.SetSessionLoad) so the server-driven ClientConfigurationManager can shift traffic across every open TableShim at once. - If session.NewClient fails, the classic pool is closed and the whole NewClientWithConfig returns the error rather than leaking a half-constructed Client. - Client.Close now closes sessionImpl (aggregating any error with the classic pool's Close error). Session backend closes FIRST — its ConfigurationManager poller and per-pool bookkeeping wind down before we drop the shared gRPC channels. - open.go: three getOrCreateSession* helpers cache per-resource session TableAPIs. Cache keys are "tbl:<t>", "av:<t>:<v>" (table- qualified so same view-id under different tables gets distinct pools + distinct metric labels), "mv:<v>". session.Client itself does NOT cache — that's this Client's responsibility. Each helper nil-short-circuits when sessionImpl == nil so classic-only callers pay zero lock cost per Open call. - OpenTable / OpenAuthorizedView / OpenMaterializedView now pass c.getOrCreateSession* result to NewTableShim instead of nil. Tests (bigtable/open_test.go, +5 new): - TestOpenTable_WithSessionBackend_WiresSessionTableAPI — verifies the shim carries a non-nil session TableAPI from sessionImpl.OpenTable. - TestOpenTable_SessionTableAPICacheHit — two OpenTable calls on the same table return the same session TableAPI pointer (identity check); sessionImpl.OpenTable is called only once. - TestOpenAuthorizedView_SessionCacheIsTableQualified — two AVs with the same view-id under DIFFERENT tables get DISTINCT session TableAPIs (defense against a bug where "av:<view>" would silently share pools across tables). - TestOpenMaterializedView_WithSessionBackend_WiresSessionTableAPI — same shape as OpenTable, plus cache-hit assertion. - TestGetOrCreateSession_NilSessionImplReturnsNil — every helper returns nil without taking the mutex when sessionImpl is nil. Existing tests (nil-session shape) still pass; the earlier five TestOpen* / TestOpenFactories tests validate the EnableSessionPool=false default. Ten open-tests total, hermetic (no dial, no emulator).
…t-session-backend # Conflicts: # bigtable/client.go # bigtable/open.go # bigtable/open_test.go
gemini review flagged that the session.NewClient-failure cleanup path called mPool.Pool.Close() directly, bypassing the ManagedChannelPool wrapper. Switch to mPool.Close() so any wrapper-owned cleanup (metrics reporter, connection recycler, dynamic scale monitor) also winds down. Matches the shape used by (*Client).Close.
…ault Removes the EnableSessionPool bool from ClientConfig. NewClientWithConfig now always calls session.NewClient — every Client carries a real session.Client and the diverter is always wired up. Motivation: - The opt-in split was cosmetic. The initial SessionLoad is 0.0, so even with the backend constructed, zero traffic hits the session data plane until a control-plane ClientConfigurationManager bumps the ratio. Callers who never see a bump pay for one session channel pool + one config-poll goroutine and no RPCs. - Removing the flag lets a control-plane update route traffic to session mid-life without a client restart or user config change, which is the whole point of server-driven traffic shaping. - One less footgun: previously an operator had to remember to flip EnableSessionPool for the traffic shaping to work; now it just does. Costs (documented on the sessionImpl field): - One extra channel pool per client (bounded, small footprint). - One background goroutine per client polling GetClientConfiguration. - Session pools + OpenSession streams remain lazily materialized on first use, so idle callers don't pay for those. Test file gets doc-comment updates; the nil-sessionImpl guard test stays (hand-built or emulator Clients can still have nil sessionImpl, and every getOrCreateSession* short-circuits on nil).
Wraps Client.sessionTables in a sessionTableCache with: - background sweeper that evicts entries idle > 1h (default TTL) - caching handle: sessionTableHandle implements session.TableAPI and IS the cache entry. Every ReadRow / MutateRow touches lastAccess automatically — no cooperation from TableShim needed. Close() on the handle removes it from the cache map + calls the underlying api.Close. - both eviction paths (caller-initiated + TTL sweep) run through handle.Close, guarded by closeOnce so double-close is safe. - newSessionTableCache accepts an injectable clock (nil → time.Now) so tests can drive eviction deterministically without racing the sweeper on c.now assignment. Before this change: sessionTables map grew monotonically for the Client's lifetime. Once opened, a resource's session TableAPI stayed in the map even if the caller stopped touching it — its pools kept running, its server-side slots stayed occupied. Bounded by (resources ever opened), not (resources currently in use). After: idle entries evict after 1h. Cache size tracks active use. Caveat, documented on the handle type: sessionTable.Close() in internal/session/table.go is a no-op today — pools tear down only when session.Client.Close fires. So the eviction wiring is complete on the bigtable side but only frees session pools once the session package grows real per-resource teardown. Follow-up work. Client.Close now closes sessionTables FIRST (stops sweeper, Close()s every remaining handle) then closes sessionImpl (drops shared channels) then mPool (classic channels). Tests (all under -race): - TestSessionTableCache_HandleIsCacheEntry — repeat getOrOpen on same key returns identical *sessionTableHandle. - TestSessionTableCache_ReadRowTouchesLastAccess — wrapper's ReadRow bumps lastAccess. - TestSessionTableCache_CloseEvictsAndFires — handle.Close removes from map + calls api.Close; second Open mints fresh handle; double-close is safe (closeOnce guards cache eviction, not api.Close — api.Close fires each call). - TestSessionTableCache_TTLSweepEvictsIdle — advance fakeClock past TTL, wait for 1ms-sweeper to fire, verify entries gone + Close()d. - TestSessionTableCache_TouchDefersEviction — touched entry stays alive across multiple half-TTL advances. - TestSessionTableCache_CloseEvictsAll — c.close() shuts sweeper and Close()s every remaining entry; idempotent. - All 10 pre-existing TestOpen*/TestGetOrCreateSession* tests still green (the cache is transparent to them).
Contributor
There was a problem hiding this comment.
Code Review
This pull request integrates a session-based data-plane client into the Bigtable client and introduces a per-resource cache (sessionTableCache) with TTL-on-idle eviction to reuse session handles. The reviewer's feedback focuses on concurrency and efficiency improvements within the cache implementation. Key recommendations include invoking the external openFn callback outside of the mutex lock using double-checked locking to prevent deadlocks, making sessionTableHandle.Close() fully idempotent, avoiding full scans during TTL sweeps by using a time-ordered list or heap, and optimizing cache shutdown to prevent redundant lock acquisition.
Comment on lines
+139
to
+156
| c.mu.Lock() | ||
| if h, ok := c.entries[key]; ok { | ||
| c.mu.Unlock() | ||
| h.touch() // touch after releasing the mutex — atomic write, no ordering issue. | ||
| return h | ||
| } | ||
| // Cache miss — open under the mutex so concurrent same-key callers | ||
| // coalesce on the same handle. session.Client.OpenTable is cheap | ||
| // (no dial), so brief mutex hold is fine. | ||
| api := c.openFn(key) | ||
| if api == nil { | ||
| c.mu.Unlock() | ||
| return nil | ||
| } | ||
| h := &sessionTableHandle{api: api, key: key, cache: c} | ||
| h.lastAccessNano.Store(c.now().UnixNano()) | ||
| c.entries[key] = h | ||
| c.mu.Unlock() |
Contributor
There was a problem hiding this comment.
Calling the external callback c.openFn(key) while holding the internal mutex c.mu violates the general rule of avoiding external callbacks under locks to prevent potential deadlocks or lock inversion. It also contradicts the documentation for newSessionTableCache which states that openFn is invoked without holding the cache mutex. Additionally, this blocks all other concurrent cache lookups (even for different keys) while openFn is executing.
We can resolve this by calling openFn outside the lock and using double-checked locking. If a duplicate handle was created concurrently, we can safely close it and return the existing one.
c.mu.Lock()
if h, ok := c.entries[key]; ok {
c.mu.Unlock()
h.touch()
return h
}
c.mu.Unlock()
api := c.openFn(key)
if api == nil {
return nil
}
c.mu.Lock()
if h, ok := c.entries[key]; ok {
c.mu.Unlock()
_ = api.Close() // Clean up duplicate
h.touch()
return h
}
h := &sessionTableHandle{api: api, key: key, cache: c}
h.lastAccessNano.Store(c.now().UnixNano())
c.entries[key] = h
c.mu.Unlock()References
- Avoid calling external callbacks while holding internal mutex locks to prevent potential deadlocks or lock inversion. If the callback is read-only after construction, invoke it before acquiring the lock.
Contributor
Author
There was a problem hiding this comment.
Fixed in fac3dd5 — refactored to double-check locking. openFn is no longer stored on the cache and no longer called under c.mu. It's a per-call parameter now (see the sushanb thread on prefix-parsing for the wider refactor).
Comment on lines
+51
to
+57
| type sessionTableHandle struct { | ||
| api session.TableAPI | ||
| key string | ||
| cache *sessionTableCache | ||
| lastAccessNano atomic.Int64 | ||
| closeOnce sync.Once | ||
| } |
Contributor
There was a problem hiding this comment.
To ensure defensive programming and safe concurrent access, we should make Close() fully idempotent by guarding the underlying api.Close() call inside closeOnce.Do as well. This prevents any potential issues (such as double-close panics or errors) in the underlying session.TableAPI implementation. We can store the close error in the handle struct.
Suggested change
| type sessionTableHandle struct { | |
| api session.TableAPI | |
| key string | |
| cache *sessionTableCache | |
| lastAccessNano atomic.Int64 | |
| closeOnce sync.Once | |
| } | |
| type sessionTableHandle struct { | |
| api session.TableAPI | |
| key string | |
| cache *sessionTableCache | |
| lastAccessNano atomic.Int64 | |
| closeOnce sync.Once | |
| closeErr error | |
| } |
Contributor
Author
There was a problem hiding this comment.
Fixed in fac3dd5 — added closeErr error field to sessionTableHandle, api.Close now inside closeOnce.Do, closeErr memoized and returned on every subsequent Close.
Comment on lines
+79
to
+84
| func (h *sessionTableHandle) Close() error { | ||
| h.closeOnce.Do(func() { | ||
| h.cache.removeEntry(h.key, h) | ||
| }) | ||
| return h.api.Close() | ||
| } |
Contributor
There was a problem hiding this comment.
Guard the underlying api.Close() call inside closeOnce.Do to ensure it is only called once, and return the cached error on subsequent calls.
Suggested change
| func (h *sessionTableHandle) Close() error { | |
| h.closeOnce.Do(func() { | |
| h.cache.removeEntry(h.key, h) | |
| }) | |
| return h.api.Close() | |
| } | |
| func (h *sessionTableHandle) Close() error { | |
| h.closeOnce.Do(func() { | |
| h.cache.removeEntry(h.key, h) | |
| h.closeErr = h.api.Close() | |
| }) | |
| return h.closeErr | |
| } |
Contributor
Author
There was a problem hiding this comment.
Fixed in fac3dd5 — Close body now runs api.Close inside closeOnce.Do, memoizes closeErr, returns closeErr on every call.
Comment on lines
+522
to
+527
| if closeCount != 2 { | ||
| // h1.Close still calls api.Close a second time — that's the | ||
| // underlying api's contract, not the cache's. closeOnce | ||
| // guards the cache eviction, not the api.Close call. | ||
| t.Errorf("underlying Close called %d times after double-Close, want 2", closeCount) | ||
| } |
Contributor
There was a problem hiding this comment.
Update the test assertion to expect the underlying Close to be called exactly once, since sessionTableHandle.Close() is now fully idempotent and guards the underlying api.Close() call.
if closeCount != 1 {
t.Errorf("underlying Close called %d times after double-Close, want 1", closeCount)
}
Contributor
Author
There was a problem hiding this comment.
Fixed in fac3dd5 — updated TestSessionTableCache_CloseEvictsAndFires to expect closeCount == 1 after double-close (fully idempotent).
Comment on lines
+196
to
+212
| func (c *sessionTableCache) sweepOnce() { | ||
| cutoff := c.now().Add(-c.ttl).UnixNano() | ||
|
|
||
| c.mu.Lock() | ||
| var evicted []*sessionTableHandle | ||
| for k, h := range c.entries { | ||
| if h.lastAccessNano.Load() < cutoff { | ||
| evicted = append(evicted, h) | ||
| delete(c.entries, k) | ||
| } | ||
| } | ||
| c.mu.Unlock() | ||
|
|
||
| for _, h := range evicted { | ||
| _ = h.Close() | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
For caches on the request path, avoid full scans for pruning expired entries. Instead of scanning the entire map in sweepOnce, use a more efficient data structure like a time-ordered list or a heap to track expirations.
References
- For caches on the request path, avoid full scans for pruning expired entries. Use more efficient data structures like a time-ordered list or a heap to track expirations.
Contributor
Author
There was a problem hiding this comment.
Not applied. Cache is not on the RPC hot path (sweep runs every 10 min by default), entry cardinality is bounded by the caller's Open* pattern (typically tens), and full-scan O(N) is cheap at that scale. Added a doc comment on sweepOnce noting the trade-off and pointing at a time-ordered heap as the future option if cardinality ever grows.
Comment on lines
+225
to
+235
| c.mu.Lock() | ||
| remaining := make([]*sessionTableHandle, 0, len(c.entries)) | ||
| for k, h := range c.entries { | ||
| remaining = append(remaining, h) | ||
| delete(c.entries, k) | ||
| } | ||
| c.mu.Unlock() | ||
|
|
||
| for _, h := range remaining { | ||
| _ = h.Close() | ||
| } |
Contributor
There was a problem hiding this comment.
Apply the same optimization here to prime h.closeOnce with a no-op function for each remaining entry. This avoids redundantly re-acquiring c.mu inside h.Close() during cache shutdown.
c.mu.Lock()
remaining := make([]*sessionTableHandle, 0, len(c.entries))
for k, h := range c.entries {
remaining = append(remaining, h)
delete(c.entries, k)
h.closeOnce.Do(func() {})
}
c.mu.Unlock()
for _, h := range remaining {
_ = h.Close()
}
Contributor
Author
There was a problem hiding this comment.
Declined. The suggested prime of closeOnce.Do(func(){}) skips h.api.Close AND leaves h.closeErr unset — that's a real behavior change (resources not released, error swallowed), not a micro-optimization. Kept the naive loop; it runs once at shutdown over what's typically a small number of handles.
sushanb
commented
Jul 29, 2026
| c.sessionTables = newSessionTableCache(func(key string) session.TableAPI { | ||
| if strings.HasPrefix(key, "tbl:") { | ||
| return sc.OpenTable(strings.TrimPrefix(key, "tbl:")) | ||
| } |
Contributor
Author
There was a problem hiding this comment.
remove this. prefix. Find a better way.
Contributor
Author
There was a problem hiding this comment.
Fixed in fac3dd5 — removed the strings.HasPrefix / TrimPrefix dispatch entirely. See the sushanb thread on open.go for the wider refactor.
sushanb
commented
Jul 29, 2026
| if c.sessionImpl == nil { | ||
| return nil | ||
| } | ||
| return c.sessionTables.getOrOpen("tbl:" + table) |
Contributor
Author
There was a problem hiding this comment.
Don't do this "tbl" find. batter way. You can keep the cache key tableResourceName,
materializedViewResourceName or authoiurzedViewNae.
Contributor
Author
There was a problem hiding this comment.
Fixed in fac3dd5. Cache is now opener-agnostic: each getOrCreateSession* helper passes its own openFn per call, and uses the fully-qualified resource name (via c.fullTableName / c.fullAuthorizedViewName / c.fullMaterializedViewName) as the cache key. Same identity Cloud Bigtable uses over the wire; no prefix parsing.
gemini (high) — sessionTableCache.getOrOpen called openFn under c.mu.
Refactored: openFn moved OFF the cache struct and BECOMES A per-call
parameter to getOrOpen. Cache now uses fast-path/slow-path with
double-check locking: fast path is a mutex read on hit; slow path
releases mutex, invokes openFn, re-acquires mutex, and either
inserts or discards a losing concurrent duplicate. openFn is never
called under c.mu.
sushanb (client.go:287, open.go:101) — no more "tbl:"/"mv:"/"av:"
prefix parsing. Cache is now opener-agnostic. Each Client
getOrCreateSession* helper passes its own openFn per call and uses
the fully-qualified resource name (projects/P/instances/I/tables/T,
etc.) as the cache key. Same identity Cloud Bigtable uses over the
wire; no collision risk across resource kinds; no dispatch logic.
gemini (medium x3) — sessionTableHandle.Close fully idempotent.
Added closeErr field; api.Close is now inside closeOnce.Do so it
fires exactly once regardless of how many callers close the handle.
Subsequent Close() calls return the memoized closeErr without
re-invoking api.Close. Test TestSessionTableCache_CloseEvictsAndFires
updated to expect close count of 1 (not 2) after double-close.
gemini (medium, sweepOnce) — deferred. Added a comment on sweepOnce
explaining why O(N) full-scan is fine here (bounded cardinality,
10-min sweep cadence, not on the RPC hot path). A time-ordered heap
would be over-engineering at this scale; noted the option for
future work.
gemini (medium, close() prime closeOnce) — declined. The suggestion
saves one mutex acquire per handle during shutdown, but the fix
requires manually priming closeOnce.Do(func(){}) which then leaves
h.closeErr wrong (never populated) and api.Close never fires — that's
a real behavior change, not a micro-optimization. Kept the naive
loop; it runs once at shutdown and iterates over what's typically a
small number of handles.
mutianf
approved these changes
Jul 29, 2026
Conformance was failing on this PR because NewClientWithConfig
unconditionally called session.NewClient. The conformance test proxy
points the Client at a fake Bigtable server that doesn't speak
GetClientConfiguration; the session-side ClientConfigurationManager
poll cascaded into a classic-side "grpc: the client connection is
closing" error on the very first Table.Apply, and the test hung
until timeout.
Re-add the EnableSessionPool bool on ClientConfig (unset by default)
and gate session.NewClient behind it. Callers who opt in get session
routing; conformance / emulator / any other fake-server harness that
doesn't flip the flag pays zero session cost and every RPC still lands
on the classic path — same behavior as before this PR series.
When EnableSessionPool is false: sessionImpl stays nil, and the
getOrCreateSession* helpers in open.go already nil-short-circuit on
that (added earlier in this PR), so Open* returns a TableShim with a
nil session and every routing decision goes classic.
When EnableSessionPool is true: unchanged from the prior commit
sequence — session.Client is constructed, AddSessionLoadListener
wires the server-driven SessionLoad into the Diverter, and
TableShims route by that ratio.
Follows the sushanb review preference from earlier in this PR ("no
session pool by default; the shim is transparent when off").
Fixes conformance (bigtable_conformance 1.25 + 1.26).
…e-session-table-cache
Piggyback on the parent PR's EnableSessionPool guard. When session backend is off, sessionImpl is nil, no session TableAPIs ever get opened, and the cache would just be a sweeper goroutine over an always-empty map. Move newSessionTableCache inside the same if-block so classic-only callers pay zero cache cost too. sessionTables stays nil in that mode; every getOrCreateSession* helper already nil-short-circuits before touching it.
…SessionPool" This reverts commit ea612a9.
This reverts commit 14bc893.
…e-session-table-cache
Fixes the conformance regression from this PR. Root cause: the conformance test proxy (bigtable/internal/testproxy/proxy.go:438-447) pre-dials one *grpc.ClientConn and hands it to NewClientWithConfig via option.WithGRPCConn(conn). Before this PR that was fine — the classic pool was the only backend and it wrapped the single conn. My session backend addition made session.NewClient ALSO wrap the same conn (opts was reused verbatim), so both backends ended up entangled on the same underlying transport. Any session-side teardown or config-poll failure against the fake server propagated to the classic ClientConn as "rpc error: code = Canceled desc = grpc: the client connection is closing" on the very first Table.Apply. Same bug hits BIGTABLE_EMULATOR_HOST paths because internal/option.DefaultClientOptions injects WithGRPCConn internally when that env var is set. Fix: detect the WithGRPCConn sentinel via internaloption.NewUnsafeResolver(opts...).ResolvedGRPCConnIsCustom() and skip session backend construction entirely in that mode. Classic path is unaffected — every RPC still lands on the pre-dialed conn as before. sessionImpl stays nil; the existing nil-short-circuit in getOrCreateSession* and Client.Close covers it. Verified locally: bigtable conformance suite runs 64/0/0 pass/fail/skip in 53s against v0.0.3 of cloud-bigtable-clients-test.
…e-session-table-cache
Propagate the parent's WithGRPCConn escape hatch to the cache: when the caller pre-dialed a conn we skip session backend construction, so the cache would just be a sweeper goroutine over an always-empty map. Move newSessionTableCache inside the same if-block. sessionTables stays nil in that mode; getOrCreateSession* already nil-short-circuits.
session.NewClient was receiving the raw caller opts instead of the fully-merged option list (o). gtransport.Dial needs the DefaultClientOptions merged in — endpoint, scopes, user-agent, interceptors, connection-pool size, direct-access options — so passing bare opts leaves the resolver target empty and the dial aborts with 'passthrough: received empty target in Build()' as soon as a caller invokes NewClientWithConfig without option.WithEndpoint. Classic path was already using o for CreateAndStartManagedChannelPool so this brings session into line and fixes end-to-end connectivity against every prod endpoint pointed at by the default.
Env-gated (CBT_RUN_SANDBOX=1) tests that exercise Client.OpenTable and Client.OpenAuthorizedView end-to-end against a real Bigtable instance, toggling the diverter between classic (SessionLoad=0.0) and session (SessionLoad=1.0) so a single test run covers both data planes with byte-identical operations. Defaults: project=autonomous-mote-782 instance=sushanb-uc1 table=sushanb family=cf12; every field env-var-overridable via CBT_SANDBOX_*. TestTableSessionSandbox — round-trips a mutation via OpenTable on both paths. TestReadNonExistentRowSandbox — probes the row-not-found → nil contract on both paths. Divergence here surfaces session-side response-parser regressions (already caught 'missing response payload' bug against sushanb-uc1). TestAuthorizedViewSessionSandbox — auto-creates a SubsetView AV 'session-test-av' (idempotent, reused across runs) then round-trips on both paths. AV path is distinct from table (different RPC, different session pool per (av, permission)), so failure here indicates independent AV-wiring regression.
… descriptors
The intermediate ReadRowResult{Row} / MutateRowResult{} wrappers were
lossy — ReadRowResult dropped SessionReadRowResponse.Stats, and
MutateRowResult had no fields to begin with — and they forced callers
to type-assert through an internal Go type even though the underlying
proto already carries everything we need.
Drop the wrappers and the decodeReadRow / decodeMutateRow helpers;
each descriptor's Decode now returns the typed envelope getter
(env.GetReadRow / env.GetMutateRow) directly. Callers type-assert to
*btpb.SessionReadRowResponse / *btpb.SessionMutateRowResponse and
read .Row / .Stats themselves, so the transport layer exposes a clean
SessionReadRowRequest -> SessionReadRowResponse contract that
downstream consumers can plug into without going through a Go-level
intermediate.
Tests updated to assert the new proto return type.
The Table and non-existent-row sandbox tests overlap with the AV test's coverage of the diverter routing path — same OpenTable-vs- OpenAuthorizedView plumbing, same classic/session toggle, same round-trip assertion. Drop them and fold the shared helpers into the AV file so it stays self-contained.
Sandbox tests belong out-of-tree — keep the branch scoped to the session-table cache and its supporting fixes.
Merged
5 tasks
sushanb
added a commit
that referenced
this pull request
Jul 30, 2026
…+ cache close-race gate (#20264) Fixes two related latent bugs in the session data plane: 1. **sessionTable.Close was a no-op** — the interface godoc at `bigtable/internal/session/api.go:45-48` promises to release the resource's read + write session pools; the implementation returned nil. Pools were reclaimed only by `sessionClient.Close`, so `bigtable.Client`'s TTL cache could evict a handle without freeing the underlying pool + streams + goroutines. 2. **sessionTableCache close-race (audit finding #5)** — a slow-path openFn straddling `sessionTableCache.close()` would install a fresh handle into a cache the sweeper had already stopped clearing. Zero-impact while sessionTable.Close was a no-op; a per-race pool leak once teardown becomes real. ## Design Real teardown routed through symmetric release closures: - `sessionClient.releaseSessionPool(key)` — under `sessionPoolsMu`, delete the entry then `unregister()` + `pool.Close()` outside the lock (matches the snapshot-under-lock pattern in `Close`). - `buildLazyReleaser(key)` — sibling to `buildLazyOpener`; returns a `func() error` closure for a specific poolKey. - `sessionTable.Close` — invokes closeRead + closeWrite, joining errors via `errors.Join`. Nil-safe for materialized views' missing write side. No refcount inside sessionClient. Rationale: `bigtable.Client`'s sessionTableCache dedupes handles per fully-qualified resource name, so at-most-one sessionTable per resource per Client at any moment — the cache is the 'refcount of at-most-1'. Doc on `sessionTable.Close` names the invariant; a future caller bypassing the cache would need to add a refcount then. Paired cache guard: `sessionTableCache` gains a `closed bool` under `c.mu`, flipped by `close()`. `getOrOpen`'s slow path re-checks it before insert and, if set, releases the freshly-opened api and returns nil so `TableShim` falls back to classic. Prevents the finding-#5 leak. ## Tests New unit tests (all pass under `-race`): - `TestSessionTable_Close_CallsBothReleasers` - `TestSessionTable_Close_NilWriteReleaserOK` (materialized view) - `TestSessionTable_Close_JoinsErrors` - `TestSessionTable_Close_ReleasersIdempotent` - `TestReleaseSessionPool_AfterClientClose_NoOp` - `TestReleaseSessionPool_MissingKeyNoOp` - `TestReleaseSessionPool_RemovesEntryAndInvokesUnregister` - `TestSessionTableCache_ClosedGate_SlowPathInsertNoLeak` ## Drive-by The pre-existing `closeCountingTable` test helper races between the sweeper goroutine's `*counter++` and the test-goroutine's read. Switched `*int` to `*atomic.Int32` so the full-package `-race` sweep is green (fires on the base branch too — this was blocking my race-stress verification). ## Test plan - [x] `go build ./...` and `go vet ./...` clean - [x] `go test -race -count=1 -short -timeout=120s ./ ./internal/session/ ./internal/transport/` — all green - [x] `TestSessionTable_Close*` and `TestReleaseSessionPool*` — all pass - [x] `TestSessionTableCache_ClosedGate_SlowPathInsertNoLeak` — reproduces the race deterministically, passes with the fix - [x] Sandbox smoke against sushanb-uc1 via `CBT_RUN_SANDBOX=1 CBT_FORCE_SESSION=true`: Table + AV round-trip on both classic and session paths ## Stack Stacked on #20263 (session_table_cache) which is stacked on #20262 (session.Client wiring into bigtable.Client Open*). Both must land first, or this PR must be rebased onto main after they merge.
sushanb
pushed a commit
that referenced
this pull request
Aug 3, 2026
🤖 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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Wraps
Client.sessionTables(a baremap[string]session.TableAPItoday) in a
sessionTableCachethat evicts entries idle for morethan a TTL (default 1 h). The wrapper
sessionTableHandleIS thecache entry and implements
session.TableAPI, so everyReadRow/MutateRowtouches lastAccess automatically — nocooperation from
TableShimneeded.Before
Client's lifetime.TableAPIstayed in the mapeven if the caller stopped touching it — its pools kept running,
its server-side slots stayed occupied.
in use).
After
Close()the returned handle explicitly forimmediate eviction.
Design — caching handle (single-type approach)
Both eviction paths (caller-initiated + TTL sweep) route through
handle.Close, guarded bycloseOnceso double-close from anycombination of paths is safe.
cache.removeEntry(key, h)deletesonly if the map still holds THIS handle — protects against a
concurrent Open that already replaced the slot.
Client.Close ordering
Close now runs in three phases:
sessionTables.close()— stops the sweeper,Close()s everyremaining handle.
sessionImpl.Close()— drops the session backend's shared gRPCchannels +
ConfigurationManagerpoller.mPool.Close()— drops the classic gRPC channels.Caveat, documented on the handle type
sessionTable.Close()inbigtable/internal/session/table.go:226-228is a no-op today — pools tear down only when
session.Client.Closefires. So the eviction wiring in this PR is complete on the bigtable
side but only frees session pools once the session package grows
real per-resource teardown. That's a follow-up on the session side.
Depends on
Client(addssessionImpl+sessionTablesfields this PR replaces the map on).Tests (all under
-race)TestSessionTableCache_HandleIsCacheEntry— repeatgetOrOpenonsame key returns identical
*sessionTableHandle.TestSessionTableCache_ReadRowTouchesLastAccess— wrapper'sReadRowbumpslastAccess.TestSessionTableCache_CloseEvictsAndFires—handle.Closeremoves from map + calls
api.Close; secondOpenmints a freshhandle; double-close safe (
closeOnceguards cache eviction, notapi.Close— which fires each call, per that call's own contract).TestSessionTableCache_TTLSweepEvictsIdle— advancefakeClockpast TTL, wait for 1 ms sweeper, verify entries gone +
Closed.TestSessionTableCache_TouchDefersEviction— touched entry staysalive across multiple half-TTL advances.
TestSessionTableCache_CloseEvictsAll—c.close()shutssweeper and
Closes every remaining entry; idempotent.TestOpen*/TestGetOrCreateSession*testsstill green (the cache is transparent to them).
Test plan
go build ./...cleango vet ./...cleangoimports -lno outputgo test ./bigtable/ -run 'TestOpen|TestGetOrCreateSession|TestSessionTableCache' -count=1 -race -v— 16/16 pass