feat(bigtable): add Session vRPC dispatch + slot lifecycle by sushanb · Pull Request #20213 · googleapis/google-cloud-go

sushanb · 2026-07-23T22:28:29Z

Summary

Second PR in a 3-PR stack layering Session behavior on top of the merged Session struct (#20117). Adds `Session.Invoke` and the one-in-flight vRPC dispatch that routes server frames back to callers.

Stacked on #20211 (Session debug surface) — this PR's counter/tracer/event writers wire up the fields introduced there.

What lands

New file: `session_vrpc.go` (~420 LOC)

`Session.Invoke` — public entry point. Serializes `claimSlot` → `Send` → `awaitInvokeResult`; deliver-vs-cancel branching lives in `routeVRPCFrame`.
Slot lifecycle: `claimSlot` / `drainSlot` / `markCancelled` / `activeVRPC` — `slotMu`-guarded, one vRPC in flight per session.
`handleVRPCResponse` / `handleVRPCErrorResponse` + shared `routeVRPCFrame` — Java-parity drain path with nil-active-VRPC + id-mismatch guards.
`deliver` — writes to the caller's `resultChan`.
`cancelActiveRPCs` — session-teardown path used by `ForceClose` / `handleGoAway` / heartbeat watchdog.
`ForceClose` — minimal port (`transitionTo(Closed) → cancelActiveRPCs → signalQuiescent`). The full lifecycle-flavored `ForceClose` (setCloseReason + notifyClosing + notifyClosed + stream cleanup) lands with the follow-up lifecycle PR.

New file: `session_vrpc_test.go` (~800 LOC) — 18 `TestInvoke*`/`TestHandleVRPC*` tests, all green under `-race`.

Edits to `session.go`

`heartbeatWake chan struct{}` field (cap-1 non-blocking) + `make(...)` in `NewSession` — reactive watchdog wake channel.
`OnSlotDrained func()` field on `SessionHooks` + `onSlotDrained()` dispatcher. Sole "session became free" signal; consumers wake pool waiters.
`Send(req)` — mutex-guarded `stream.Send` wrapper. Used by this PR's `Invoke` and the follow-up lifecycle `Start`.
`resetHeartbeatDeadline` + `wakeHeartbeatLoop` — atomic-based reactive wake pair; consumed by the follow-up lifecycle `heartBeatLoop`.

Edits to `session_test.go`

Test fixtures: `fakeStream` / `fakeDesc` / `newFakeStream` / `newRoundTripDesc` / `makeActive` / `waitFor`. Consumed by both this PR's tests and the follow-up lifecycle tests.
`newTestSession` signature change (`stream, hooks`); pre-existing 1-arg call sites now use `newDefaultTestSession` shortcut.

Edits to `debug_tracer.go`

`tagSessionVRPCCancelledDrained` — bookkeeping tag when a server response drains a slot whose caller already `ctx.Done`'d.

Stack

feat(bigtable): add Session debug surface (observability fields + methods) #20211 — Session debug surface (merged into origin/bigtable-session-debug)
This PR — Session vRPC dispatch + slot lifecycle
Next — `session_lifecycle.go` (Start, Close, readLoop, heartBeatLoop, handleSessionResponse/OpenSession/GoAway/Close/ErrorResponse, peerInfoExtracter). Will extend `ForceClose` to its full lifecycle-shaped body.

Test plan

`go build ./internal/transport/` passes
`go vet ./internal/transport/` clean
`go test ./internal/transport/ -race -count=1 -short -timeout 90s` passes — 18 new `TestInvoke*`/`TestHandleVRPC*` tests plus all pre-existing tests

gemini-code-assist

Code Review

This pull request introduces virtual RPC (vRPC) dispatching, slot lifecycle management, and enhanced observability/debugging features for Bigtable transport sessions. It adds the sessionDebug struct to consolidate observability metrics, implements the Invoke method for executing vRPCs, and integrates a new OnSlotDrained lifecycle hook. The review feedback highlights two key issues: a bug in the synchronous Send failure path where hooks are triggered unconditionally regardless of whether drainSlot succeeded, and a potential nil pointer dereference when accessing res.resp fields during response processing.

gemini-code-assist · 2026-07-23T22:31:41Z

+		s.drainSlot(rpc)
+		// v3: drain is the sole "session became free" signal. Fire the
+		// pool wake here as well — ReleaseToPool's inExpectedCount guard
+		// (see SESSION_POOL_SPEC #6, I5) drops the re-enqueue if
+		// OnSessionClosing already dropped this handle in the teardown
+		// that the Send failure typically kicks off.
+		s.hooks.onSlotDrained()
+		if State(s.state.Load()) == StateClosing {
+			s.signalQuiescent()
+		}


In the synchronous Send failure path, s.drainSlot(rpc) is called, but its return value is ignored. If a concurrent teardown (such as cancelActiveRPCs or ForceClose) has already run and drained the slot, drainSlot will return ok == false. Unconditionally calling s.hooks.onSlotDrained() and s.signalQuiescent() in this case violates the design where teardown paths do not trigger onSlotDrained (which could cause the pool to incorrectly re-enqueue a closing/closed session or wake waiters on a dead session). We should only execute these hooks if drainSlot actually succeeded (i.e., returned ok == true).

Suggested change

s.drainSlot(rpc)

// v3: drain is the sole "session became free" signal. Fire the

// pool wake here as well — ReleaseToPool's inExpectedCount guard

// (see SESSION_POOL_SPEC #6, I5) drops the re-enqueue if

// OnSessionClosing already dropped this handle in the teardown

// that the Send failure typically kicks off.

s.hooks.onSlotDrained()

if State(s.state.Load()) == StateClosing {

s.signalQuiescent()

}

if _, _, ok := s.drainSlot(rpc); ok {

// v3: drain is the sole "session became free" signal. Fire the

// pool wake here as well — ReleaseToPool's inExpectedCount guard

// (see SESSION_POOL_SPEC #6, I5) drops the re-enqueue if

// OnSessionClosing already dropped this handle in the teardown

// that the Send failure typically kicks off.

s.hooks.onSlotDrained()

if State(s.state.Load()) == StateClosing {

s.signalQuiescent()

}

}

References

Avoid broadcasting unconditionally on the failure path if no slots are actually freed.

Applied in 16047d6 — guarded both onSlotDrained and signalQuiescent on drainSlot's ok return. Idempotence isn't the point; the design says teardown paths do NOT fire onSlotDrained (SessionHooks docstring on session.go:157-166 in the debug-surface PR, SESSION_SPEC #5/#10). The unconditional fire on the Send-failure loser path violated that. Thanks for catching.

Traced the race on the parent branch (feat/bigtable-sessionz-debug) where the full pool machinery is present. The ok=false path gemini flagged is fully absorbed by two invariants that live on the pool side:

notifyClosing runs synchronously before cancelActiveRPCs on every teardown path (ForceClose at session_lifecycle.go:102/109, handleClose at :475/494, handleGoAway at :434, heartbeat miss at :641). So when B (cancelActiveRPCs) wins the drainSlot race, hooks.onClosing → p.sl.OnSessionClosing has already flipped inExpectedCount = false and removed sh from afe.sessions (session_list.go:307-326, dropMembershipLocked at :498-503).

ReleaseToPool early-returns on !sh.inExpectedCount (session_list.go:285-305 — pinned by TestSessionList_ReleaseToPool_AfterClosing_NoOp at session_list_test.go:428-461). So A's hooks.onSlotDrained → ReleaseToPool on the ok=false iteration is a no-op re-enqueue. signalFree wakes a waiter, which re-picks via ReadyAfes — the closing session is already gone from that set, so the waiter never receives the dying handle.

So on the parent branch, the guard we added here elides only a spurious wake — no correctness delta.

BUT: this PR is scoped to the vRPC layer alone. ReleaseToPool.inExpectedCount lands with the pool PR later in the stack. Until that PR lands upstream, the guard here is load-bearing defense. Keeping it in place; will revisit removing it when the pool-side dedup arrives.

Correction to my previous reply: reverting the guard in 14644803fa.

After re-tracing on the parent branch, the guard is redundant even on this PR's stack once you follow the emission chain end-to-end:

Teardown ordering. notifyClosing → hooks.onClosing → p.sl.OnSessionClosing runs synchronously before cancelActiveRPCs on every teardown path (ForceClose at session_lifecycle.go:102/109, handleClose at :475/494, handleGoAway at :434, heartbeat miss at :641). So when B (cancelActiveRPCs) wins the drainSlot race, inExpectedCount = false is already published and the handle is already gone from afe.sessions (session_list.go:307-326 + dropMembershipLocked).

ReleaseToPool guard. session_list.go:285-305 early-returns on !sh.inExpectedCount. A's unguarded onSlotDrained on the ok=false iteration is a no-op re-enqueue.

signalFree wake. Waiters re-pick via ReadyAfes (session_pool.go:236-249) — the closing session is out of that set, so the waiter never receives the dying handle.

Emission stays dumb; the pool absorbs duplicates at ReleaseToPool. The TestSessionList_ReleaseToPool_AfterClosing_NoOp pin at session_list_test.go:428-461 protects the guard from regression.

Keeping the design consistent with the parent branch so we don't have to re-guard when this rebases.

gemini-code-assist · 2026-07-23T22:31:41Z

+	if res.errResp != nil {
+		return tagErr(StateServerResult, errorResponseToErr(res.errResp))
+	}
+	respMsg, decodeErr := desc.Decode(res.resp.Payload)


To prevent potential nil pointer dereferences, we should defensively check if res.resp is nil before accessing its fields (such as Payload and Stats).

Suggested change

respMsg, decodeErr := desc.Decode(res.resp.Payload)

if res.resp == nil {

return tagErr(StateServerResult, fmt.Errorf("vRPC response payload is missing"))

}

respMsg, decodeErr := desc.Decode(res.resp.Payload)

Not needed — the vrpcResult union is constructed by deliver and routeVRPCFrame such that exactly one of {resp, errResp, err} is populated per frame. routeVRPCFrame's id-mismatch guard runs before drainSlot, so a matching-id delivery either carries a valid resp (success path from handleVRPCResponse) or falls into the errResp/err branches. processResult matches on the union kind and never touches .resp.Payload without knowing resp != nil. Adding a nil check would suggest an invariant we're not confident about — the code path is proof it can't be nil here.

sushanb

igor-reviewer-agent

Went through af844b2f13. Slot-lifecycle plumbing reads clean, the three-drain-site symmetry with onSlotDrained is well-motivated, and the ForceCloseWhileSending test is a nice piece of coverage. A handful of small things below — nothing blocking on wire correctness, mostly readability nits and one flake risk.

session_vrpc.go:awaitInvokeResult (ctx.Done branch)
recordCtxDone calls activeVRPC() which takes slotMu, then markCancelled takes slotMu again a couple lines later. nit, fold into one shot — either return the stillActive snapshot from markCancelled, or do the debug read inside markCancelled while youre already holding the lock.

session_vrpc.go:buildInvokeRequest
nit, ctx first per Go convention:

func buildInvokeRequest(ctx context.Context, rpcID int64, reqBytes []byte, attempt int64, startTime time.Time) *spb.SessionRequest {

session_vrpc.go:deliver
nit, its a one-line wrapper around a buffered chan send. either inline it at the two call sites or drop it — the docstring is doing more work than the function is. If youre keeping it for the future-instrumentation hook, thats fine, just say so.

session_vrpc.go:processResult

_ = rpc // rpc kept in signature for future per-rpc metrics; no reads today.

please just drop the arg until you need it. _ = rpc reads as dead weight and a future reader will wonder what they missed.

session_vrpc_test.go:TestInvoke_ForceCloseWhileSending_BoundedReturn

time.Sleep(20 * time.Millisecond)
close(sendGate)

20ms sleeps in a race test are asking for CI flakes under load — the whole point of the test is race behaviour and youre hoping the scheduler cooperates. Can this be a sync signal instead? eg have ForceClose fire a hook / signal that cancelActiveRPCs drained, then close the gate. If you really cant get a deterministic signal, at least bump the sleep to something like 100ms and add a comment about the flake budget.

session_vrpc.go:Invoke — heartbeat reset placement

s.resetHeartbeatDeadline()
...
if sendErr := s.Send(sessionReq); sendErr != nil {

why reset before Send returns? If Send fails on a broken stream we've pushed the watchdog for a frame that never made it. I dont have strong feelings about it since the session tears down on TransportFailure anyway, but folding it into the success branch would be more honest about what actually got on the wire.

LGTM w/ nits — none of the above blocks merge, but the ForceCloseWhileSending sleep is worth a follow-up before this hits CI on a busy day.

…Loop, heartBeatLoop) Third and final PR in the Session core stack. Adds the lifecycle orchestration that drives a Session from Start through teardown, plus the readLoop that dispatches server frames to the vRPC handlers landed in bigtable-session-vrpc. Stacks on bigtable-session-vrpc; deletes the minimal ForceClose stub that PR shipped, replacing it with the full lifecycle-shaped body. New file: session_lifecycle.go (~640 LOC) - Session.Start(ctx, OpenSessionRequest) — transitions New→Starting, Sends the OpenSession frame, fires onStart, spawns readLoop + heartBeatLoop. Wraps a failed Send as codes.Unavailable so retry plumbing treats pre-wire OpenSession loss the same as any other transport-side loss. - ForceClose — full body: transitionTo(Closed), setCloseReason, notifyClosing (once), cancelActiveRPCs, signalQuiescent, notifyClosed. - Close(ctx, CloseSessionRequest) — graceful drain: Ready→Closing, waits on quiescent, sends CloseSession, transitions to WaitServerClose, arms the pool's stuck-session sweep to eventually ForceClose if the server never confirms. - notifyClosing / notifyClosed — once-guarded hook dispatchers with the strict onClosing-precedes-onClose ordering enforced. - readLoop(ctx) — Recv-loop that dispatches every SessionResponse variant via handleSessionResponse, drives handleClose on stream termination, records msgsRecv counters + resets heartbeat deadline on every recognized frame. - handleSessionResponse — dispatch switch to handleOpenSession / handleVRPCResponse / handleErrorResponse / handleSessionParameters / handleGoAway / handleSessionRefreshConfig. Heartbeat frames update the deadline via resetHeartbeatDeadline; the unknown-payload branch records a debug tag but doesn't reset the deadline (so a misbehaving server can't keep the watchdog satisfied with junk). - handleOpenSession — Starting→Ready transition, PeerInfo extract from the bidi header (synchronous, matches Java's onHeaders synchrony), fires onActive. - handleGoAway / handleClose / handleErrorResponse / handleSessionParameters / handleSessionRefreshConfig — protocol-level handlers. - heartBeatLoop — Timer + heartbeatWake reactive-wake pair. Only enforces the deadline while a vRPC is in flight (idle sessions legitimately receive no server heartbeats). See SESSION_SPEC.md #7. - peerInfoExtracter — parses the bigtable-peer-info header, stamps s.peerInfo atomically before onActive fires. - closeReasonLabel / closeReasonToCause — CloseSessionRequest.Reason → string / sentinel error mapping for close-reason attribution. New file: session_lifecycle_test.go (~660 LOC) — 30+ tests covering Start, ForceClose, Close, readLoop, handleSessionResponse dispatch, handleOpenSession + peerInfo parsing, handleGoAway, handleClose, handleErrorResponse (rpc_id=0 harmlessly drops via routeVRPCFrame guards), heartBeatLoop (reactive wake, idle-gate, missed-heartbeat ForceClose). Edits to session_vrpc.go - Delete the minimal ForceClose stub introduced in the prior PR — the full body lives in session_lifecycle.go now. Edits to session_test.go - Add hookCounts helper (start/active/close callback counters). - Add setSlotForTest helper (seed the in-flight slot from tests). Stack: - googleapis#20211 — Session debug surface - googleapis#20213 — Session vRPC dispatch + slot lifecycle - This PR — Session lifecycle

Five nits from the igor-reviewer-agent review of PR googleapis#20213: - Return stillActive bool from markCancelled and use it in awaitInvokeResult so the ctx.Done branch does one slotMu take instead of two (markCancelled + recordCtxDone's activeVRPC). - Move ctx to the first arg of buildInvokeRequest per Go convention. - Drop the dead `_ = rpc` in processResult; remove rpc from the signature (only awaitInvokeResult calls it, no rpc reads today). - Delete the deliver() one-line wrapper — inline the buffered send at both call sites (routeVRPCFrame's non-cancelled branch, cancelActiveRPCs's non-cancelled branch). Two-word "cap-1, drainSlot serialized this write" comment replaces the paragraph docstring. - Replace time.Sleep(20ms) in TestInvoke_ForceCloseWhileSending_ BoundedReturn with a deterministic waitFor on activeVRPC() == nil, so the test doesn't rely on scheduler cooperation. Also converts the two remaining string-literal call sites in noteRetryAttempt and recordCtxDone to the typed SessionEventKind constants (SessionEventRetry, SessionEventCtxDone) landed in the prior PR-1 nit pass.

…Loop, heartBeatLoop) Third and final PR in the Session core stack. Adds the lifecycle orchestration that drives a Session from Start through teardown, plus the readLoop that dispatches server frames to the vRPC handlers landed in bigtable-session-vrpc. Stacks on bigtable-session-vrpc; deletes the minimal ForceClose stub that PR shipped, replacing it with the full lifecycle-shaped body. New file: session_lifecycle.go (~640 LOC) - Session.Start(ctx, OpenSessionRequest) — transitions New→Starting, Sends the OpenSession frame, fires onStart, spawns readLoop + heartBeatLoop. Wraps a failed Send as codes.Unavailable so retry plumbing treats pre-wire OpenSession loss the same as any other transport-side loss. - ForceClose — full body: transitionTo(Closed), setCloseReason, notifyClosing (once), cancelActiveRPCs, signalQuiescent, notifyClosed. - Close(ctx, CloseSessionRequest) — graceful drain: Ready→Closing, waits on quiescent, sends CloseSession, transitions to WaitServerClose, arms the pool's stuck-session sweep to eventually ForceClose if the server never confirms. - notifyClosing / notifyClosed — once-guarded hook dispatchers with the strict onClosing-precedes-onClose ordering enforced. - readLoop(ctx) — Recv-loop that dispatches every SessionResponse variant via handleSessionResponse, drives handleClose on stream termination, records msgsRecv counters + resets heartbeat deadline on every recognized frame. - handleSessionResponse — dispatch switch to handleOpenSession / handleVRPCResponse / handleErrorResponse / handleSessionParameters / handleGoAway / handleSessionRefreshConfig. Heartbeat frames update the deadline via resetHeartbeatDeadline; the unknown-payload branch records a debug tag but doesn't reset the deadline (so a misbehaving server can't keep the watchdog satisfied with junk). - handleOpenSession — Starting→Ready transition, PeerInfo extract from the bidi header (synchronous, matches Java's onHeaders synchrony), fires onActive. - handleGoAway / handleClose / handleErrorResponse / handleSessionParameters / handleSessionRefreshConfig — protocol-level handlers. - heartBeatLoop — Timer + heartbeatWake reactive-wake pair. Only enforces the deadline while a vRPC is in flight (idle sessions legitimately receive no server heartbeats). See SESSION_SPEC.md #7. - peerInfoExtracter — parses the bigtable-peer-info header, stamps s.peerInfo atomically before onActive fires. - closeReasonLabel / closeReasonToCause — CloseSessionRequest.Reason → string / sentinel error mapping for close-reason attribution. New file: session_lifecycle_test.go (~660 LOC) — 30+ tests covering Start, ForceClose, Close, readLoop, handleSessionResponse dispatch, handleOpenSession + peerInfo parsing, handleGoAway, handleClose, handleErrorResponse (rpc_id=0 harmlessly drops via routeVRPCFrame guards), heartBeatLoop (reactive wake, idle-gate, missed-heartbeat ForceClose). Edits to session_vrpc.go - Delete the minimal ForceClose stub introduced in the prior PR — the full body lives in session_lifecycle.go now. Edits to session_test.go - Add hookCounts helper (start/active/close callback counters). - Add setSlotForTest helper (seed the in-flight slot from tests). Stack: - googleapis#20211 — Session debug surface - googleapis#20213 — Session vRPC dispatch + slot lifecycle - This PR — Session lifecycle

Applies the same nits accepted on PRs googleapis#20211/googleapis#20213/googleapis#20215 to the sessionz working branch. See per-PR commits cf200ae / c51caae / 1f8a958 for rationale. Plus the ForceClose(nil) spurious-tag fix from 35d3b16.

Three golint vet errors reported by CI on PR googleapis#20213: - session_vrpc.go:202: var virtRpc should be virtRPC (initialisms uppercase per Go convention). - session_vrpc_test.go:37,138: runVRpcCapture and runInvoke had (t *testing.T, ctx context.Context, ...) — vet requires ctx as the first parameter. Rename to (ctx, t, ...) and update all callers (6 total across the file). No behavior change.

…Loop, heartBeatLoop) Third and final PR in the Session core stack. Adds the lifecycle orchestration that drives a Session from Start through teardown, plus the readLoop that dispatches server frames to the vRPC handlers landed in bigtable-session-vrpc. Stacks on bigtable-session-vrpc; deletes the minimal ForceClose stub that PR shipped, replacing it with the full lifecycle-shaped body. New file: session_lifecycle.go (~640 LOC) - Session.Start(ctx, OpenSessionRequest) — transitions New→Starting, Sends the OpenSession frame, fires onStart, spawns readLoop + heartBeatLoop. Wraps a failed Send as codes.Unavailable so retry plumbing treats pre-wire OpenSession loss the same as any other transport-side loss. - ForceClose — full body: transitionTo(Closed), setCloseReason, notifyClosing (once), cancelActiveRPCs, signalQuiescent, notifyClosed. - Close(ctx, CloseSessionRequest) — graceful drain: Ready→Closing, waits on quiescent, sends CloseSession, transitions to WaitServerClose, arms the pool's stuck-session sweep to eventually ForceClose if the server never confirms. - notifyClosing / notifyClosed — once-guarded hook dispatchers with the strict onClosing-precedes-onClose ordering enforced. - readLoop(ctx) — Recv-loop that dispatches every SessionResponse variant via handleSessionResponse, drives handleClose on stream termination, records msgsRecv counters + resets heartbeat deadline on every recognized frame. - handleSessionResponse — dispatch switch to handleOpenSession / handleVRPCResponse / handleErrorResponse / handleSessionParameters / handleGoAway / handleSessionRefreshConfig. Heartbeat frames update the deadline via resetHeartbeatDeadline; the unknown-payload branch records a debug tag but doesn't reset the deadline (so a misbehaving server can't keep the watchdog satisfied with junk). - handleOpenSession — Starting→Ready transition, PeerInfo extract from the bidi header (synchronous, matches Java's onHeaders synchrony), fires onActive. - handleGoAway / handleClose / handleErrorResponse / handleSessionParameters / handleSessionRefreshConfig — protocol-level handlers. - heartBeatLoop — Timer + heartbeatWake reactive-wake pair. Only enforces the deadline while a vRPC is in flight (idle sessions legitimately receive no server heartbeats). See SESSION_SPEC.md #7. - peerInfoExtracter — parses the bigtable-peer-info header, stamps s.peerInfo atomically before onActive fires. - closeReasonLabel / closeReasonToCause — CloseSessionRequest.Reason → string / sentinel error mapping for close-reason attribution. New file: session_lifecycle_test.go (~660 LOC) — 30+ tests covering Start, ForceClose, Close, readLoop, handleSessionResponse dispatch, handleOpenSession + peerInfo parsing, handleGoAway, handleClose, handleErrorResponse (rpc_id=0 harmlessly drops via routeVRPCFrame guards), heartBeatLoop (reactive wake, idle-gate, missed-heartbeat ForceClose). Edits to session_vrpc.go - Delete the minimal ForceClose stub introduced in the prior PR — the full body lives in session_lifecycle.go now. Edits to session_test.go - Add hookCounts helper (start/active/close callback counters). - Add setSlotForTest helper (seed the in-flight slot from tests). Stack: - googleapis#20211 — Session debug surface - googleapis#20213 — Session vRPC dispatch + slot lifecycle - This PR — Session lifecycle

Bundles four related changes for the sessionz working branch: 1) 1x heartbeat multiplier (was 3x). resetHeartbeatDeadline and handleSessionParameters now compute `now + interval` instead of `now + 3*interval`. Matches java-bigtable's SessionImpl.java:440 (nextHeartbeat = clock.instant().plus(heartbeatInterval)). Under healthy operation nothing changes — each frame's reset fires before the timer wakes. Only sessions where the server genuinely goes silent for one interval trip the watchdog. 2) heartBeatLoop cleanup: - Delete `active := 1 // multiplex=1; kept as %d so a future >1 stays greppable` code smell (variable existed only for its self-documenting comment). - lastFrameAge = interval - remaining (was 3*interval - remaining) to match the new 1x deadline. - Debugf/recordEvent format strings drop the in_flight=%d field that was carrying the constant 1. 3) Vet fixes matching PR googleapis#20213's CI signal: - virtRpc → virtRPC (initialism convention). - runVRpcCapture and runInvoke: ctx moves to first parameter per Go convention. Updated all 5 + 5 callers in the test file. 4) SESSION_SPEC.md #7: update the "Deadline = 3 × heartbeatInterval" bullet to reflect the new 1x semantics and default interval. Tests: - TestHandleSessionParameters_UpdatesIntervalAndDeadline: expect deadline ~= before+interval (1x) instead of before+3*interval. - TestHeartBeatLoop_HeartbeatsKeepInflightVRPCAlive: seed deadline at now+30ms (1x) instead of now+90ms (was 3x).

sushanb · 2026-07-24T00:39:14Z

@igor-reviewer-agent thanks — all 6 nits addressed. SHA breakdown:

c51caae6a3 (nits):

markCancelled returns stillActive bool; awaitInvokeResult uses it to skip the second slotMu take that was happening via recordCtxDone(...activeVRPC())
buildInvokeRequest ctx-first per Go convention
processResult drops the dead _ = rpc param entirely
deliver() deleted, inlined at both call sites
time.Sleep(20 * time.Millisecond) in TestInvoke_ForceCloseWhileSending_BoundedReturn replaced with waitFor(t, time.Second, activeVRPC()==nil, ...) — deterministic instead of scheduler-hoping

adceb099c0 (separate concern surfaced by another reviewer, mentioning here for context): resetHeartbeatDeadline dropped 3× multiplier → 1× to match SessionImpl.java:440.

9fda75dae8 (vet green): virtRpc → virtRPC and ctx-first in the two test helpers you didn't flag but vet did.

On the resetHeartbeatDeadline firing pre-Send "why?": kept as-is intentionally — an outbound frame is client-side activity that resets the server's keepalive expectation regardless of whether the Send succeeds. If Send fails, the session tears down anyway on TransportFailure, so the deadline being pushed 100ms too far has no observable consequence.

Introduces Session.Invoke and the one-in-flight vRPC dispatch that routes server frames back to callers. Stacks on the sessionDebug bundle (bigtable-session-debug); the counter/tracer/event writers land here. New file: session_vrpc.go (~420 LOC) - Session.Invoke — public entry point. Serializes claimSlot → Send → awaitInvokeResult; deliver-vs-cancel branching lives in routeVRPCFrame. - Slot lifecycle: claimSlot / drainSlot / markCancelled / activeVRPC — slotMu-guarded, one vRPC in flight per session. - handleVRPCResponse / handleVRPCErrorResponse + shared routeVRPCFrame — Java-parity drain path with nil-active-VRPC + id-mismatch guards. - deliver — writes to the caller's resultChan. - cancelActiveRPCs — session-teardown path used by ForceClose / handleGoAway / heartbeat watchdog. - ForceClose — minimal port (transitionTo(Closed) → cancelActiveRPCs → signalQuiescent). The full lifecycle-flavored ForceClose (setCloseReason + notifyClosing + notifyClosed + stream cleanup) lands with the session_lifecycle follow-up. Edits to session.go - Add heartbeatWake chan struct{} field (cap-1 non-blocking) + make(...) in NewSession — reactive watchdog wake channel. - Add OnSlotDrained func() to SessionHooks + onSlotDrained() dispatcher. Sole "session became free" signal; consumers wake pool waiters. - Add Send(req) — mutex-guarded stream.Send wrapper; used by both this PR's Invoke and the follow-up lifecycle Start. - Add resetHeartbeatDeadline + wakeHeartbeatLoop — atomic-based reactive wake pair; consumed by the follow-up lifecycle heartBeatLoop. Edits to session_test.go - Test fixtures: fakeStream / fakeDesc / newFakeStream / newRoundTripDesc / makeActive / waitFor. Consumed by both this PR's TestInvoke*/TestHandleVRPC* and the follow-up lifecycle tests. - newTestSession signature change (stream, hooks); pre-existing 1-arg call sites now use newDefaultTestSession shortcut. Edits to debug_tracer.go - tagSessionVRPCCancelledDrained — bookkeeping tag when a server response drains a slot whose caller already ctx.Done'd. Stack: - Prereq: PR googleapis#20211 (bigtable-session-debug) — Session debug surface. - Follow-up: session_lifecycle.go (Start, Close, readLoop, heartBeatLoop, handleSessionResponse / handleOpenSession / handleGoAway / handleClose / handleErrorResponse / peerInfoExtracter). Will extend ForceClose to its full lifecycle-shaped body.

Address gemini-code-assist review on session_vrpc.go:153-162: the Send- failure branch was firing hooks.onSlotDrained() and signalQuiescent() unconditionally, ignoring drainSlot's ok return. When concurrent teardown (cancelActiveRPCs / ForceClose) beats the Invoke path to freeing the slot, the teardown path is responsible for pool notification via OnClosing/OnClose — firing onSlotDrained on the losing path violates the SessionHooks contract that reserves it for the drain-succeeded path (SESSION_SPEC #5/#10). Wrap both calls in `if _, _, ok := s.drainSlot(rpc); ok { ... }`. Idempotence of the underlying ops isn't the point — the design says teardown paths do NOT fire onSlotDrained, and this brings the Send-failure branch back in line.

…ents Twelve doc-comment references to Java SessionImpl line-number citations and "Java parity" phrasing rewritten to describe Go behavior directly. No code change.

Five nits from the igor-reviewer-agent review of PR googleapis#20213: - Return stillActive bool from markCancelled and use it in awaitInvokeResult so the ctx.Done branch does one slotMu take instead of two (markCancelled + recordCtxDone's activeVRPC). - Move ctx to the first arg of buildInvokeRequest per Go convention. - Drop the dead `_ = rpc` in processResult; remove rpc from the signature (only awaitInvokeResult calls it, no rpc reads today). - Delete the deliver() one-line wrapper — inline the buffered send at both call sites (routeVRPCFrame's non-cancelled branch, cancelActiveRPCs's non-cancelled branch). Two-word "cap-1, drainSlot serialized this write" comment replaces the paragraph docstring. - Replace time.Sleep(20ms) in TestInvoke_ForceCloseWhileSending_ BoundedReturn with a deterministic waitFor on activeVRPC() == nil, so the test doesn't rely on scheduler cooperation. Also converts the two remaining string-literal call sites in noteRetryAttempt and recordCtxDone to the typed SessionEventKind constants (SessionEventRetry, SessionEventCtxDone) landed in the prior PR-1 nit pass.

Java-bigtable's SessionImpl.java sets nextHeartbeat = now + heartbeatInterval at every reset (SessionImpl.java:440 in startRpc, :621 in handleHeartBeatResponse). Our 3x multiplier was a Go-only invention — tolerated 2 additional missed heartbeats before force-closing, but that extra grace comes at the cost of stale-session detection latency and diverges from the reference implementation. Under the new 1x deadline, a healthy session with regular server frames never trips the watchdog (each frame's resetHeartbeatDeadline pushes the deadline forward). Only sessions whose server has genuinely stopped sending frames for one full interval fire ForceClose — the actual "heartbeat missed" signal the watchdog exists to detect. Follow-up in session_lifecycle.go's heartBeatLoop will adjust its lastFrameAge = 3*interval - remaining arithmetic to match.

Three golint vet errors reported by CI on PR googleapis#20213: - session_vrpc.go:202: var virtRpc should be virtRPC (initialisms uppercase per Go convention). - session_vrpc_test.go:37,138: runVRpcCapture and runInvoke had (t *testing.T, ctx context.Context, ...) — vet requires ctx as the first parameter. Rename to (ctx, t, ...) and update all callers (6 total across the file). No behavior change.

Reverts commit 971e4ec. After tracing the race gemini flagged against the parent sessionz branch where the full pool machinery is in scope, the guard is redundant: - notifyClosing runs synchronously before cancelActiveRPCs on every teardown path (ForceClose, handleClose, handleGoAway, heartbeat miss). By the time cancelActiveRPCs wins drainSlot, the pool has already flipped inExpectedCount=false and removed the handle from afe.sessions via OnSessionClosing. - ReleaseToPool early-returns on !sh.inExpectedCount, so an unguarded onSlotDrained on the ok=false iteration is a no-op re-enqueue. - signalFree wakes a waiter which re-picks via ReadyAfes — the closing session is already out of that set, so the waiter never receives the dying handle. Emit stays dumb here; the pool absorbs dupes at ReleaseToPool.

mutianf · 2026-07-27T01:16:24Z

+	// Claim the single in-flight slot. A losing claim means a prior vRPC
+	// on this session has not drained on the wire yet — return Uncommitted
+	// so the retryer picks another session.
+	if !s.claimSlot(rpc) {


If this happens, would it indicate race condition somewhere in the code? Session picker should only pick a ready and idle session to server the traffic? And we only get 3 retries for uncommitted errors. Would this cause problem with a burst of traffic?

Traced through this. Under the current pool sequencing, this branch is defensive rather than reachable: a session is only added to the AFE idle queue after OnSlotDrained fires (session_list.go:ReleaseToPool), and markCancelled deliberately does NOT fire that hook — so a ctx.Done'd caller leaves the slot claimed and the session stays OUT of the idle set until the server response drains it. The pool's Checkout only dequeues sessions currently in the idle set (guarded by sl.mu), so two callers can't get the same session. If we ever DO hit this branch, retrying is correct in both possible causes: (a) a pool sequencing bug (retry on another session avoids the broken one), (b) a legitimate teardown race (retry with fresh session). The 3-retry cap isn't burst-amplified — if we systematically hit this, the same defect fires again on retry regardless of QPS. Keeping the guard as defense-in-depth. Fixed in 85c0aeb.

mutianf · 2026-07-27T01:32:07Z

+	}
+	respMsg, decodeErr := desc.Decode(res.resp.Payload)
+	if decodeErr != nil {
+		return tagErr(StateServerResult, fmt.Errorf("decode vRPC response: %w", decodeErr))


Hmm I think decodeErr should be transport error instead of server result?

Applied in 85c0aeb — actually deferred and reverted the initial flip. First pass I moved decodeErr to StateTransportFailure per your ask, but session-reviewer caught it against SESSION_SPEC #9: the server sent a response, so the request LIKELY committed even if we can't parse the reply. Under StateTransportFailure, an idempotent-only-under-strict-ID mutation would retry and risk double-apply. Kept at StateServerResult with a comment citing the double-apply risk. Callers with a genuine idempotency token can override at the retry-oracle level.

wouldn't serverResult with no retry info also leads to immediate retry? If something is wrong and we're not able to decode the message, would this leads this RPC to be retried forever (until operation timeout?)

mutations are idempotent if:

there's no aggregates mutations (addToCell / mergeCell)

setCell mutations don't use server timestamps (-1)

So even if we failed to decode a mutate row response as long as it's idempotent it's still safe to retry? and we can limit it to retry 3 times?

I agree though that transport error might also not be the right category, I don't think we have a category for this error, but it's definetly not a server error? 🤔

mutianf · 2026-07-27T01:47:24Z

+// deliver already fired the terminal error; nothing to do.
+func (s *Session) routeVRPCFrame(rpcID int64, frameName, nilTag string, counter *atomic.Int64, result vrpcResult) {
+	st := s.State()
+	if !assertDebugTagf(st == StateReady || st == StateClosing, tagSessionVRPCResponseWrongState,


Should we fail loud on these scenarios? wrong state, rpc is null and rpc id mismatch? Silently dropping the frame seems dangerous. should we at least tear down the session in this case? error level logging and at least return the wrong state / nil/ mismatch error to the caller so the listener is notified?

Applied in 85c0aeb. Three-way split matching the semantics of each drop:

wrong state (New/Starting/Closed): unrecoverable state-tracking inconsistency. Emit SessionEventProtocolError + ForceClose(CLOSE_SESSION_REASON_ERROR) — transitions to Closed, drains active slot via cancelActiveRPCs, and (once pool-side machinery lands) fires OnClosing so the pool removes the session from AFE routing.

rpc_id mismatch: genuine server desync. Same escalation as above — caller receives a terminal Unavailable via cancelActiveRPCs; session leaves the pool cleanly.

nil active vRPC: legit ctx.Done race (caller cancelled locally; server's response arrived after). NOT a protocol violation. Tagged with new SessionEventLateFrame kind — separate from ProtocolError so operators grep-filtering desync alerts don't see benign late-frame drops swamping the signal. Session stays Ready.

Also widened the state whitelist to include WaitServerClose (the drain window between our CloseSession send and server's EOF legitimately delivers late vRPC frames). Test coverage: 5 pinning tests including status.Code + session state + event-ring kind assertions on each of the three drop scenarios plus Closing and WaitServerClose whitelist pins.

…calation Three tightly-scoped changes on session_vrpc.go per mutianf's review on googleapis#20213: 1. Decode failure classified as StateTransportFailure (was StateServerResult). A malformed payload means we couldn't consume the wire bytes, so no server-application-level result exists to reason about; a retry (potentially on another session/AFE) may succeed if the bad payload was a one-off from a specific backend. 2. Broadened the vRPC-frame state whitelist to accept WaitServerClose. The drain window between our CloseSession send and the server's EOF legitimately delivers late VirtualRpc responses; treating them as wrong-state produced false protocol errors during graceful teardown. 3. Replaced silent debug-tag drops on the three routeVRPCFrame guard paths with a new SessionEventProtocolError event kind + escalation: - wrong state (New/Starting/Closed): tear down via cancelActiveRPCs - nil active vRPC (post-cancel race): log, drop, keep session - rpc_id mismatch (server desync): tear down via cancelActiveRPCs Silent drops on protocol violations left the current caller waiting for a response that would never arrive; escalating to cancelActiveRPCs delivers a terminal error so the retry oracle picks another session/AFE instead of hitting ctx timeout. Three new tests cover the routing behaviors: - TestHandleVRPCResponse_IDMismatch_TearsDownSession - TestHandleVRPCResponse_WrongState_TearsDownSession - TestHandleVRPCResponse_NilRPC_DoesNotTearDown Legacy behavior (silent drop into debug tag) preserved only for the nil-active-vRPC case because that path is a documented race between local ctx.Done cancel and a legitimate late server response.

Addresses mutianf's review on PR googleapis#20213 (backport of the identical change on bigtable-session-vrpc). Silent debug-tag drops on routeVRPCFrame's three guard paths left callers waiting on ctx timeout AND left desynced sessions in AFE rotation. Behavior: * State ∈ {Ready, Closing, WaitServerClose} — expected. Whitelist broadened to include WaitServerClose: the drain window between our CloseSession send and the server's EOF legitimately delivers late vRPC frames. * State ∈ {New, Starting, Closed} — unrecoverable state-tracking inconsistency. Record SessionEventProtocolError, then ForceClose with reason ERROR so the OnClosing/OnClose hooks fire and the pool removes the session from AFE routing. In practice this branch is unreachable in production (readLoop only runs post-Start; ForceClose no-ops on Closed) but kept as belt-and-suspenders. * activeVRPC()==nil — legit ctx.Done race. Tagged with a NEW SessionEventLateFrame kind (separate from ProtocolError so operators grep-filtering "protocol-error" don't see benign late-frame drops). Frame dropped, session stays Ready. * rpc_id mismatch — genuine server desync. Record SessionEventProtocolError + ForceClose(ERROR). Caller receives a terminal Unavailable via cancelActiveRPCs (fired from within ForceClose); the retry oracle steers to a different session/AFE instead of hitting ctx timeout. Escalation uses ForceClose rather than cancelActiveRPCs alone because the latter is a teardown-companion (SESSION_SPEC #2: cancelActiveRPCs intentionally SKIPS OnSlotDrained; the pool's removal-from-routing comes from OnClosing/OnClose). ForceClose gives us both: OnClosing fires so the pool drops the session from the AFE queue, AND cancelActiveRPCs runs internally so the parked caller wakes. decodeErr classification kept at StateServerResult per SESSION_SPEC #9 — server likely committed the request even if we can't parse the reply, so a retry on non-idempotent mutations risks double-apply. Reverted a prior tempting flip to StateTransportFailure. SESSION_SPEC #2 updated to match: the stale "dropped, not delivered (canary counter; unreachable in normal production)" text is replaced with the ForceClose-escalation description, the OnClosing → AFE removal chain, the ProtocolError vs LateFrame kind split, and the "no longer unreachable" note. Test coverage: * TestHandleVRPCResponse_IDMismatch_TearsDownSession — pins the id-mismatch escalation. Asserts status.Code == Unavailable, session state == Closed, and event ring contains SessionEventProtocolError. * TestHandleVRPCResponse_WrongState_TearsDownSession — pins the wrong-state escalation from StateStarting (Closed early-returns from ForceClose and the caller is by definition already unblocked). * TestHandleVRPCResponse_NilRPC_DoesNotTearDown — pins the nil-active ctx.Done race: session stays Ready, event ring gets LateFrame (NOT ProtocolError), so the kind split is protected against regression. * TestHandleVRPCResponse_ClosingState_AcceptsMatchingFrame — pins Closing in the whitelist. * TestHandleVRPCResponse_WaitServerCloseState_AcceptsMatchingFrame — pins WaitServerClose in the whitelist (the widening this commit introduces).

…rr classification Follow-up to f4ab4fa addressing session-reviewer's two spec violations + Igor's nits. Same code as feat/bigtable-sessionz-debug 0a51296. Behavior deltas from f4ab4fa: 1. decodeErr classification REVERTED from StateTransportFailure back to StateServerResult. The prior flip widened the retry surface: an idempotent write whose response bytes were unreadable would have retried under transport-failure rules, risking double-apply. Server sent a response → request likely committed even if we can't parse the reply. Callers with a genuine idempotency token can override at the retry-oracle level. 2. Wrong-state and rpc_id-mismatch paths now call ForceClose( CLOSE_SESSION_REASON_ERROR) instead of cancelActiveRPCs alone. cancelActiveRPCs is a teardown-companion: it drains the slot but does NOT fire OnClosing/OnClose, so a session escalated via cancelActiveRPCs-alone stays Ready and orphaned from the pool's AFE routing set (once that pool-side machinery lands in a later PR — on this PR it's a no-op distinction but keeps the discipline aligned with the future pool wiring). ForceClose additionally marks the session Closed so subsequent Invokes on the same session-handle see state!=Ready and return Uncommitted for the retry oracle. 3. Split SessionEventProtocolError vs SessionEventLateFrame: - ProtocolError: wrong-state (New/Starting/Closed) + id-mismatch (genuine server desync). Both escalated via ForceClose. - LateFrame: nil-active-vRPC — legit ctx.Done race, session stays Ready. Separate kind so operators grep-filtering "protocol-error" don't see benign late-frame drops swamping desync alerts. Test coverage expanded to 5 tests: * TestHandleVRPCResponse_IDMismatch_TearsDownSession — asserts status.Code == Unavailable, state == Closed, event ring contains ProtocolError. * TestHandleVRPCResponse_WrongState_TearsDownSession — uses StateStarting (Closed early-returns from ForceClose CAS; the caller in Closed state is by definition already unblocked). * TestHandleVRPCResponse_NilRPC_DoesNotTearDown — session stays Ready, event ring has LateFrame, ProtocolError absent (kind split regression pin). * TestHandleVRPCResponse_ClosingState_AcceptsMatchingFrame — pins Closing in the whitelist (drain window must accept matching frames). * TestHandleVRPCResponse_WaitServerCloseState_AcceptsMatchingFrame — pins the whitelist widening the parent commit introduced.

…Loop, heartBeatLoop) Third and final PR in the Session core stack. Adds the lifecycle orchestration that drives a Session from Start through teardown, plus the readLoop that dispatches server frames to the vRPC handlers landed in bigtable-session-vrpc. Stacks on bigtable-session-vrpc; deletes the minimal ForceClose stub that PR shipped, replacing it with the full lifecycle-shaped body. New file: session_lifecycle.go (~640 LOC) - Session.Start(ctx, OpenSessionRequest) — transitions New→Starting, Sends the OpenSession frame, fires onStart, spawns readLoop + heartBeatLoop. Wraps a failed Send as codes.Unavailable so retry plumbing treats pre-wire OpenSession loss the same as any other transport-side loss. - ForceClose — full body: transitionTo(Closed), setCloseReason, notifyClosing (once), cancelActiveRPCs, signalQuiescent, notifyClosed. - Close(ctx, CloseSessionRequest) — graceful drain: Ready→Closing, waits on quiescent, sends CloseSession, transitions to WaitServerClose, arms the pool's stuck-session sweep to eventually ForceClose if the server never confirms. - notifyClosing / notifyClosed — once-guarded hook dispatchers with the strict onClosing-precedes-onClose ordering enforced. - readLoop(ctx) — Recv-loop that dispatches every SessionResponse variant via handleSessionResponse, drives handleClose on stream termination, records msgsRecv counters + resets heartbeat deadline on every recognized frame. - handleSessionResponse — dispatch switch to handleOpenSession / handleVRPCResponse / handleErrorResponse / handleSessionParameters / handleGoAway / handleSessionRefreshConfig. Heartbeat frames update the deadline via resetHeartbeatDeadline; the unknown-payload branch records a debug tag but doesn't reset the deadline (so a misbehaving server can't keep the watchdog satisfied with junk). - handleOpenSession — Starting→Ready transition, PeerInfo extract from the bidi header (synchronous, matches Java's onHeaders synchrony), fires onActive. - handleGoAway / handleClose / handleErrorResponse / handleSessionParameters / handleSessionRefreshConfig — protocol-level handlers. - heartBeatLoop — Timer + heartbeatWake reactive-wake pair. Only enforces the deadline while a vRPC is in flight (idle sessions legitimately receive no server heartbeats). See SESSION_SPEC.md #7. - peerInfoExtracter — parses the bigtable-peer-info header, stamps s.peerInfo atomically before onActive fires. - closeReasonLabel / closeReasonToCause — CloseSessionRequest.Reason → string / sentinel error mapping for close-reason attribution. New file: session_lifecycle_test.go (~660 LOC) — 30+ tests covering Start, ForceClose, Close, readLoop, handleSessionResponse dispatch, handleOpenSession + peerInfo parsing, handleGoAway, handleClose, handleErrorResponse (rpc_id=0 harmlessly drops via routeVRPCFrame guards), heartBeatLoop (reactive wake, idle-gate, missed-heartbeat ForceClose). Edits to session_vrpc.go - Delete the minimal ForceClose stub introduced in the prior PR — the full body lives in session_lifecycle.go now. Edits to session_test.go - Add hookCounts helper (start/active/close callback counters). - Add setSlotForTest helper (seed the in-flight slot from tests). Stack: - googleapis#20211 — Session debug surface - googleapis#20213 — Session vRPC dispatch + slot lifecycle - This PR — Session lifecycle

…Loop, heartBeatLoop) (#20215) ## Summary Third and final PR in the Session core stack. Adds the lifecycle orchestration that drives a Session from \`Start\` through teardown, plus the \`readLoop\` that dispatches server frames to the vRPC handlers landed in #20213. **Stacks on** #20213 (Session vRPC dispatch + slot lifecycle). Deletes the minimal \`ForceClose\` stub that PR shipped, replacing it with the full lifecycle-shaped body. ### What lands **New file: \`session_lifecycle.go\`** (~640 LOC) - \`Session.Start(ctx, OpenSessionRequest)\` — transitions New→Starting, Sends the OpenSession frame, fires \`onStart\`, spawns \`readLoop\` + \`heartBeatLoop\`. Wraps a failed \`Send\` as \`codes.Unavailable\` so retry plumbing treats pre-wire OpenSession loss the same as any other transport-side loss. - \`ForceClose\` — full body: \`transitionTo(Closed)\` → \`setCloseReason\` → \`notifyClosing\` (once) → \`cancelActiveRPCs\` → \`signalQuiescent\` → \`notifyClosed\`. - \`Close(ctx, CloseSessionRequest)\` — graceful drain: Ready→Closing, waits on \`quiescent\`, sends CloseSession, transitions to WaitServerClose, arms the pool's stuck-session sweep to eventually ForceClose if the server never confirms. - \`notifyClosing\` / \`notifyClosed\` — once-guarded hook dispatchers with the strict \`onClosing\`-precedes-\`onClose\` ordering enforced. - \`readLoop(ctx)\` — Recv-loop that dispatches every SessionResponse variant via \`handleSessionResponse\`, drives \`handleClose\` on stream termination, records \`msgsRecv\` counters + resets heartbeat deadline on every recognized frame. - \`handleSessionResponse\` — dispatch switch to \`handleOpenSession\` / \`handleVRPCResponse\` / \`handleErrorResponse\` / \`handleSessionParameters\` / \`handleGoAway\` / \`handleSessionRefreshConfig\`. Heartbeat frames reset the deadline; unknown-payload branch records a debug tag but doesn't reset (so a misbehaving server can't keep the watchdog satisfied with junk). - \`handleOpenSession\` — Starting→Ready transition, PeerInfo extract from the bidi header (synchronous, matches Java's onHeaders synchrony), fires \`onActive\`. - \`handleGoAway\` / \`handleClose\` / \`handleErrorResponse\` / \`handleSessionParameters\` / \`handleSessionRefreshConfig\` — protocol-level handlers. - \`heartBeatLoop\` — Timer + \`heartbeatWake\` reactive-wake pair. Only enforces the deadline while a vRPC is in flight (idle sessions legitimately receive no server heartbeats). See SESSION_SPEC.md #7. - \`peerInfoExtracter\` — parses the \`bigtable-peer-info\` header, stamps \`s.peerInfo\` atomically before \`onActive\` fires. - \`closeReasonLabel\` / \`closeReasonToCause\` — CloseSessionRequest.Reason → string / sentinel error mapping for close-reason attribution. **New file: \`session_lifecycle_test.go\`** (~660 LOC) — 30+ tests covering Start, ForceClose, Close, readLoop, handleSessionResponse dispatch, handleOpenSession + peerInfo parsing, handleGoAway, handleClose, handleErrorResponse (rpc_id=0 harmlessly drops via routeVRPCFrame guards), heartBeatLoop (reactive wake, idle-gate, missed-heartbeat ForceClose). **Edits to \`session_vrpc.go\`** - Delete the minimal \`ForceClose\` stub introduced in the prior PR — the full body lives in \`session_lifecycle.go\` now. **Edits to \`session_test.go\`** - \`hookCounts\` helper (start/active/close callback counters). - \`setSlotForTest\` helper (seed the in-flight slot from tests). ### Stack 1. #20211 — Session debug surface 2. #20213 — Session vRPC dispatch + slot lifecycle 3. **This PR** — Session lifecycle ## Test plan - [x] \`go build ./internal/transport/\` passes - [x] \`go vet ./internal/transport/\` clean - [x] \`go test ./internal/transport/ -count=1 -short -timeout 90s\` passes — 30+ new lifecycle tests plus all pre-existing tests

…#20224) ## Summary First of five PRs porting the session-pool infrastructure from `feat/bigtable-sessionz-debug` (which powers the recycle-repro fleet) upstream. Stacks on #20215 (Session lifecycle, now merged). ### What lands **New file: `session_list.go`** (~600 LOC) — the per-pool sessionList data structure that groups sessions by the AFE (Application Front End) their handshake landed on. Consumed by the two-tier picker (K-choice-over-AFEs → dequeue-idle-session) in a follow-up PR. Key types: - `AfeID` (int64) — AFE identifier from the server's PeerInfo header at session-open. 0 is the sentinel "unknown" bucket for handshakes that did not carry a peer-info header. - `AfeSnapshot` — value-typed view of an afeHandle for pickers to score without holding sl.mu (Checkout re-resolves by ID; no *afeHandle escapes). - `AfeSnapshotRow` — debug-UI row emitted by `sessionList.Snapshot()` (consumed by afez/sessionz in a later debug PR). - `afeHandle` (unexported) — per-AFE bucket: FIFO idle queue, refCount (idle + inFlight + closing), two PeakEwma trackers. - `SessionHandle` — pool bookkeeping wrapper around Session. Carries `inExpectedCount` (I5 guard against WaitServerClose retry storm) and `activated / closingRecorded / closeRecorded` dedup flags for the pool's per-session hook chain. - `sessionList` (unexported) — the state machine, guarded by `sl.mu`. The state model documents **six invariants (I1-I6)** that every method preserves: ``` I1 inExpectedCount ⇒ handleToAfe[sh] != nil I2 readyCount == count of inExpectedCount handles I3 afesWithReady == {afe : len(afe.sessions) > 0} I4 afe.refCount == count of handleToAfe entries pointing at afe I5 sh in afe.sessions ⇒ handleToAfe[sh]==afe AND inExpectedCount I6 refCount-- only on OnSessionClosed (Closing keeps slot warm) ``` Lock order: `sl.mu` ONLY. `RecordVRpcOutcome` deliberately drops `sl.mu` between the map lookup and the `PeakEwma.Update` so the hot vRPC-outcome path doesn't serialize on it. Consolidates AFE types (`AfeID` + `AfeSnapshot`) that previously lived in `afe_snapshot.go` — sessionList now owns all AFE-bucket concepts. Deletes `afe_snapshot.go`. **New file: `session_list_test.go`** (~770 LOC) — I1-I6 coverage plus per-method tests (OnSessionStarted / Checkout / ReleaseToPool / OnSessionClosing / OnSessionClosed / RecordVRpcOutcome / ReadyAfes / Snapshot / AllHandles / Prune) and a concurrency stress test covering the documented lock-drop path in RecordVRpcOutcome. **Edits to `debug_tracer.go`** — three new tag constants for sessionList bookkeeping violations (all unreachable-under-invariants, kept as belt-and-suspenders): - `tagSessionListStartedNilSession` - `tagSessionListRefcountUnderflow` - `tagSessionListReadyCountUnderflow` **Edit to `session.go`** — one-line comment retarget on `AfeID()` (type now lives in `session_list.go`, not the deleted `afe_snapshot.go`). ### Stack 1. #20211 — Session debug surface (merged) 2. #20213 — Session vRPC dispatch (merged) 3. #20215 — Session lifecycle (merged) 4. **This PR** — sessionList (PR-1 of 5) 5. Next — SessionPoolImpl core, pool_lifecycle, pool_scaling, pool_debug ## Test plan - [x] `go build ./internal/transport/` passes - [x] `go vet ./internal/transport/` clean - [x] `go test ./internal/transport/ -race -count=1 -short -timeout=120s` passes — 20+ new sessionList tests plus all pre-existing.

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

sushanb requested review from a team as code owners July 23, 2026 22:28

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

gemini-code-assist Bot reviewed Jul 23, 2026

View reviewed changes

sushanb mentioned this pull request Jul 23, 2026

feat(bigtable): add Session lifecycle (Start, Close, ForceClose, readLoop, heartBeatLoop) #20215

Merged

3 tasks

sushanb changed the base branch from main to bigtable-session-debug July 23, 2026 22:35

sushanb force-pushed the bigtable-session-debug branch from 2534c88 to 9c553f6 Compare July 23, 2026 22:46

sushanb force-pushed the bigtable-session-vrpc branch 2 times, most recently from 16047d6 to af844b2 Compare July 23, 2026 23:07

sushanb mentioned this pull request Jul 23, 2026

feat(bigtable): add Session debug surface (observability fields + methods) #20211

Merged

3 tasks

sushanb commented Jul 23, 2026

View reviewed changes

sushanb force-pushed the bigtable-session-vrpc branch from af844b2 to c51caae Compare July 23, 2026 23:44

sushanb changed the base branch from bigtable-session-debug to main July 24, 2026 21:56

sushanb added 6 commits July 24, 2026 21:57

docs(bigtable): drop java-parity references from session_vrpc.go comm…

3c061e7

…ents Twelve doc-comment references to Java SessionImpl line-number citations and "Java parity" phrasing rewritten to describe Go behavior directly. No code change.

sushanb force-pushed the bigtable-session-vrpc branch from 9fda75d to a206193 Compare July 24, 2026 21:57

mutianf reviewed Jul 27, 2026

View reviewed changes

sushanb closed this Jul 27, 2026

sushanb reopened this Jul 27, 2026

sushanb mentioned this pull request Jul 27, 2026

feat(bigtable): add per-AFE sessionList for the two-tier session pool #20224

Merged

3 tasks

sushanb mentioned this pull request Jul 27, 2026

feat(bigtable): add SessionPoolImpl (two-tier pool + scaling + debug) #20225

Merged

8 tasks

Uh oh!

Conversation

sushanb commented Jul 23, 2026

Summary

What lands

Stack

Test plan

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 23, 2026

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!

gemini-code-assist Bot Jul 23, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sushanb left a comment

Choose a reason for hiding this comment

Uh oh!

sushanb commented Jul 24, 2026

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants