fix(bigtable): real per-resource pool teardown on sessionTable.Close + cache close-race gate by sushanb · Pull Request #20264 · googleapis/google-cloud-go

sushanb · 2026-07-30T05:18:01Z

Fixes two related latent bugs in the session data plane:

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.
sessionTableCache close-race (audit finding storage: bucket management #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

go build ./... and go vet ./... clean
go test -race -count=1 -short -timeout=120s ./ ./internal/session/ ./internal/transport/ — all green
TestSessionTable_Close* and TestReleaseSessionPool* — all pass
TestSessionTableCache_ClosedGate_SlowPathInsertNoLeak — reproduces the race deterministically, passes with the fix
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.

gemini-code-assist

Code Review

This pull request introduces a per-resource session table cache (sessionTableCache) with TTL-on-idle eviction to cache session TableAPI handles (for tables, authorized views, and materialized views) in the Cloud Bigtable Go client. It integrates this cache into the client and TableShim to dynamically route traffic based on server-driven session load, and implements proper teardown of session pools upon handle or client closure. Review feedback identifies a potential race condition between lazy openers and Close() that could leak session pools, and suggests adding defensive nil checks for session pool entries, the openFn callback, and the cache receiver in removeEntry.

gemini-code-assist · 2026-07-30T05:20:54Z

 func (t *sessionTable) Close() error {
-	return nil
+	var errs []error
+	if t.closeRead != nil {
+		if err := t.closeRead(); err != nil {
+			errs = append(errs, err)
+		}
+	}
+	if t.closeWrite != nil {
+		if err := t.closeWrite(); err != nil {
+			errs = append(errs, err)
+		}
+	}
+	return errors.Join(errs...)
 }


There is a potential race condition between lazy openers and Close(). If a read or write operation is initiated (triggering the lazy opener), and concurrently Close() is called (e.g., due to a timeout or cache eviction), Close() will call releaseSessionPool() which finds the pool is not yet in sc.sessionPools and no-ops. Once the lazy opener finishes dialing, it will insert the newly created pool into sc.sessionPools, resulting in a leaked session pool that will never be closed. Consider tracking the closed state in sessionTable and coordinating with the lazy openers to prevent this leak.

References

Prioritize correctness and proper resource release over micro-optimizations, particularly in non-performance-critical paths like shutdown.

Ensure cleanup operations, such as waiting for background tasks to finish, are executed in all exit paths of a Close or cleanup function, even if the operation's context is already canceled.

Fixed in 96a5caa. Added atomic.Bool closed on sessionTable; Close() stores true BEFORE running releasers. The lazy openers are wrapped by guardOpen which (a) early-bails on closed, (b) re-checks after the underlying opener returns and invokes the release closure itself if closed=true — cleaning up any pool that got inserted while Close's own releaser was no-op-ing on an empty map. Two deterministic tests (TestSessionTable_Close_RacingLazyOpen_NoLeak + TestSessionTable_Close_BeforeLazyOpen_EarlyBail) pin the two interleavings.

gemini-code-assist · 2026-07-30T05:20:55Z

+	if mp.unregister != nil {
+		mp.unregister()
+	}
+	return mp.pool.Close()


To adhere to defensive programming practices and prevent potential nil pointer dereference panics, we should ensure that mp and mp.pool are non-nil before accessing their fields or calling their methods. Since sc.sessionPools can theoretically store nil pointers or uninitialized pools, adding explicit guards is highly recommended.

if mp == nil { return nil } if mp.unregister != nil { mp.unregister() } if mp.pool != nil { return mp.pool.Close() } return nil

Declining: sc.sessionPools is only written by getOrCreateSessionPool (client.go:773) which constructs mp := &managedSessionPool{pool: pool} with a real pool before insert — never nil, never partial. Same for mp.unregister (nil is the tested sentinel for 'no unregister needed', already handled by the if mp.unregister != nil check on line 680). Adding defensive nil-guards on invariants that construction guarantees would (a) be dead code and (b) mask a real bug if one ever inserted nil (silent no-op instead of a nil-panic that surfaces the caller error). Keeping the current shape.

The preDialed detection at NewClientWithConfig was reading from the raw caller opts, but BIGTABLE_EMULATOR_HOST injects option.WithGRPCConn inside btopt.DefaultClientOptions (bigtable/internal/option/option.go:113) — the emulator conn only lives in the merged option list (o), never in opts. Reading only from opts missed the emulator conn, session.NewClient tried to dial an empty resolver target, and every emulator-based test failed with 'passthrough: received empty target in Build()'. Repro: TestIntegration_NewClientWithEmulatorHost (integration_test.go:495). Post-fix that test passes.

sessionTable.Close was a no-op today because session pools are shared via sessionClient.sessionPools[poolKey] and were only reclaimed by sessionClient.Close. That drift meant bigtable.Client's TTL cache could evict a handle without freeing its underlying session pool + streams + goroutines. Design: add sessionClient.releaseSessionPool(key) and a buildLazyReleaser helper symmetric with the existing buildLazyOpener. OpenTable / OpenAuthorizedView / OpenMaterializedView now pass close closures to newSessionTable; sessionTable.Close invokes both (nil-safe for materialized views' missing write side) and joins any errors via errors.Join. releaseSessionPool no-ops cleanly when sessionPools is nil (client already closed) or when the key is absent (idempotent), matching the snapshot-under-lock + teardown-outside pattern used by sessionClient.Close. No refcount added: bigtable.Client's sessionTableCache guarantees at-most-one sessionTable per resource at any moment, so the cache IS the 'refcount of at-most-1'. Doc on sessionTable.Close names this invariant; if a future caller bypasses the cache the design must gain a refcount then. Api.go doc updated to match. Also fixes audit finding #5 (cache-close race): sessionTableCache gains a closed flag flipped under mu by close(). getOrOpen slow path re-checks it before insert — a slow openFn straddling close() now releases its freshly-opened api and returns nil (TableShim treats nil session as classic-only) instead of orphaning a handle whose Close would never fire. Without this the freshly-real teardown work above would turn every close-race into a per-race session-pool leak. Drive-by: switch the closeCountingTable test helper's counter from *int to *atomic.Int32. The sweeper-vs-test-goroutine race on that counter is pre-existing (fires under full-package -race on the base branch too) but was blocking the race-stress verification for this change.

Adds the observability surface on top of PR googleapis#20264: 7 debug views (sessionz, afez, loadz, channelz, configz, tcpz, debugtagsz), per-conn TCP_INFO scraping, and the plumbing that makes them work. Public contract: - ClientConfig.EnableClientDebug bool (default false) - Client.TCPStats() *TCPStats — nil when EnableClientDebug is false - Client.SessionDebug() / ChannelDebug() / ConfigDebug() — the three provider accessors debugview.Handler consumes When EnableClientDebug is true, NewClientWithConfig auto-constructs a TCPStats collector and appends its grpc.WithContextDialer option to the dial list, so every managed gRPC connection lands in the per-conn registry that /debug/tcpz/ renders. The same flag is threaded into session.NewClient so per-pool + per-session recorders (pickHistory, latencyHist, event rings) stay off when nobody will consume them. Wiring: mux.Handle("/debug/", http.StripPrefix("/debug", debugview.Handler(client, client.TCPStats()))) Non-goals: - No //go:build sessionz compile-time exclusion — the code is always linked; the flag gates the recorders. - No changes to Client.Close ordering — tcpStats needs no explicit shutdown (gRPC conn teardown drains the registry via GC). - No changes to ChannelPoolSnapshot on this branch — dropped the Instance/AppProfile columns from channelz's pool header rather than plumb new fields into connpool.go. Reconciliation: - session.NewClient gains an enableDebug bool positional param. Sole caller (bigtable.NewClientWithConfig) updated. - session_debug.go accessor short-circuits on sessionImpl==nil (upstream has no EnableSessionPool flag on this branch). - AfeID renamed from unexported afeID upstream — debugview conversions added at the map/struct-literal boundary. Tests: build+vet clean, -race sweep green across ./ ./internal/session/ ./internal/transport/ ./debugview/.

…imports fixup Addresses PR googleapis#20264 review comment (high, gemini-code-assist): If a read or write operation is initiated (triggering the lazy opener), and concurrently Close() is called, Close() will call releaseSessionPool() which finds the pool is not yet in sc.sessionPools and no-ops. Once the lazy opener finishes dialing, it will insert the newly created pool into sc.sessionPools, resulting in a leaked session pool. The concrete bad interleaving: T1: readPool.get() enters slow path in getOrCreateSessionPool (constructs pool, about to insert under sessionPoolsMu). T2: sessionTable.Close() -> closeRead -> releaseSessionPool(readKey) finds map empty, no-op. T1: inserts fresh pool. Nobody holds a Close handle on it. Leak until bigtable.Client.Close drains sessionPools. Fix: sessionTable gains an atomic.Bool 'closed'. Close() flips it BEFORE the releasers run. The lazy openers (readPool.open, writePool.open) are wrapped by guardOpen, which: 1. Early check: if closed, return ErrClientClosed without dialing. 2. Post-check after the underlying opener returns: if now closed, invoke the release closure directly to tear down our own insert. releaseSessionPool is idempotent so Close's parallel release call harmlessly no-ops. The at-most-one-sessionTable-per-resource cache invariant still underpins per-handle teardown safety — the fix only closes the race INSIDE a single handle's lifetime; the doc on Close names both. Tests (deterministic, no timing dependence): - TestSessionTable_Close_RacingLazyOpen_NoLeak: reproduces the exact interleaving via chan-blocked openRead + concurrent Close, asserts release was called twice and get() returned ErrClientClosed. - TestSessionTable_Close_BeforeLazyOpen_EarlyBail: verifies the early check short-circuits without invoking the opener. Drive-by: goimports -w on table_test.go stripped a trailing blank line (fixes the vet CI failure on the PR).

mutianf · 2026-07-30T17:10:54Z

 	resource := fmt.Sprintf("av:%s:%s", table, view)
+	readKey := poolKey{resource, permissionRead}
+	writeKey := poolKey{resource, permissionWrite}
 	openRead := sc.buildLazyOpener(fullName, btransport.AUTHORIZED_VIEW_SESSION, streamFactory,


IIRC this won't immediately close the pool, instead it'll wait for traffic to actually open it right? this is just creating a placeholder?

🤖 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 30, 2026 05:18

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

gemini-code-assist Bot reviewed Jul 30, 2026

View reviewed changes

sushanb added 2 commits July 30, 2026 05:22

sushanb force-pushed the fix/bigtable-sessiontable-close-teardown branch from 195cfaf to 8018b52 Compare July 30, 2026 05:23

mutianf approved these changes Jul 30, 2026

View reviewed changes

sushanb merged commit 599aea9 into googleapis:main Jul 30, 2026
19 checks passed

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

chore(main): release bigtable 1.52.0 #20207

Merged

Navigation Menu

Uh oh!

fix(bigtable): real per-resource pool teardown on sessionTable.Close + cache close-race gate - #20264

fix(bigtable): real per-resource pool teardown on sessionTable.Close + cache close-race gate#20264
sushanb merged 3 commits into
googleapis:mainfrom
sushanb:fix/bigtable-sessiontable-close-teardown

sushanb commented Jul 30, 2026

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

gemini-code-assist Bot Jul 30, 2026

Uh oh!

sushanb Jul 30, 2026

Uh oh!

gemini-code-assist Bot Jul 30, 2026

Uh oh!

sushanb Jul 30, 2026

Uh oh!

Uh oh!

Uh oh!

mutianf Jul 30, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

sushanb commented Jul 30, 2026

Design

Tests

Drive-by

Test plan

Stack

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Jul 30, 2026

Choose a reason for hiding this comment

Uh oh!

sushanb Jul 30, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jul 30, 2026

Choose a reason for hiding this comment

Uh oh!

sushanb Jul 30, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mutianf Jul 30, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants