Skip to content

READY feat(bigtable) : add dynamic channel support - #2

Merged
sushanb merged 23 commits into
mainfrom
go2
Dec 4, 2025
Merged

READY feat(bigtable) : add dynamic channel support#2
sushanb merged 23 commits into
mainfrom
go2

Conversation

@sushanb

@sushanb sushanb commented Nov 3, 2025

Copy link
Copy Markdown
Owner

No description provided.

Comment thread bigtable/internal/transport/connpool.go Outdated
Comment thread bigtable/internal/transport/connpool.go Outdated
Comment on lines +644 to +648
newConns := make([]*connEntry, numCurrent+n)
copy(newConns, currentConns)
copy(newConns[numCurrent:], newEntries[:n])
p.conns.Store(newConns)
btopt.Debugf(p.logger, "bigtable_connpool: Added %d connections, new size: %d\n", n, len(newConns))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems to still have the same problem as we discussed ... you are exposing unprimed channels to the pool.

How about adding a wait group for all of the goroutines and waiting on all of the to finish?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

Comment thread bigtable/internal/transport/connpool.go Outdated
Comment thread bigtable/internal/transport/connpool.go Outdated
index int
}

// removeConnections returns true if the pool size changed.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please expand the comment explaining which connections will be targeted for removal

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

expanded.

Comment thread bigtable/internal/transport/connpool.go
Comment thread bigtable/internal/transport/connpool.go Outdated
Comment thread bigtable/internal/transport/connpool.go Outdated

dynamicConfig btopt.DynamicChannelPoolConfig // Keep the config for options
// background monitors
monitors []Monitor

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The relationship between a pool and a monitor is very murky. It seems like a Monitor should know about a pool and control the pool. But for some reason the pool owns the references to the monitor? Why not have the client own struct own it and have a cleaner separation?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--> Why not have the client own struct own it and have a cleaner separation?

yeah it is murky.

what do you prefer?

  1. client owning the reference to the monitor and controlling the pool.

  2. or passing the functions instead of the entire pool reference to the monitor? similar to the MetricsExporter

NewDynamicScaleMonitor(
pool.dynamicConfig,
pool.ConnectionLoadStatsSuppolier,
pool.addConnections,
pool.removeConnections,
)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

made it part of client.

Comment thread bigtable/internal/transport/dynamic_scale_monitor.go Outdated
Comment thread bigtable/internal/transport/dynamic_scale_monitor.go Outdated
Comment thread bigtable/internal/transport/dynamic_scale_monitor.go Outdated
Comment thread bigtable/internal/transport/dynamic_scale_monitor.go Outdated
Comment thread bigtable/internal/transport/dynamic_scale_monitor.go Outdated
Comment thread bigtable/internal/transport/dynamic_scale_monitor.go Outdated
@sushanb
sushanb changed the base branch from go1 to main November 16, 2025 23:28
@sushanb sushanb changed the title NOT_READY feat(bigtable) : add dynamic channel support READY feat(bigtable) : add dynamic channel support Nov 17, 2025
@sushanb
sushanb merged commit 8579aed into main Dec 4, 2025
sushanb added a commit that referenced this pull request Dec 21, 2025
* WIP

* WIO

* Fix

* Address feedback

* remove alts

* FIX

* WIO

* WIP

* WIP

* Make conn draining timeout 30mins

* WIP

* WIP

* WIP

* WIP

* WIP

* WIP

* WIP

* WIP

* Review comments

* WIP

* WIP

* go2 complete
sushanb added a commit that referenced this pull request Dec 21, 2025
* WIP

* WIO

* Fix

* Address feedback

* remove alts

* FIX

* WIO

* WIP

* WIP

* Make conn draining timeout 30mins

* WIP

* WIP

* WIP

* WIP

* WIP

* WIP

* WIP

* WIP

* Review comments

* WIP

* WIP

* go2 complete
sushanb added a commit to googleapis/google-cloud-go that referenced this pull request Dec 22, 2025
sushanb pushed a commit that referenced this pull request Feb 6, 2026
…3782)

PR created by the Librarian CLI to initialize a release. Merging this PR
will auto trigger a release.

Librarian Version: v0.8.0
Language Image:
us-central1-docker.pkg.dev/cloud-sdk-librarian-prod/images-prod/librarian-go@sha256:01189c9771ac4150742aed38eb52e19a008018889066002742034b7f82db070f
<details><summary>bigtable: 1.42.0</summary>

##
[1.42.0](googleapis/google-cloud-go@bigtable/v1.41.0...bigtable/v1.42.0)
(2026-02-04)

### Features

* add TieredStorageConfig to table admin api (PiperOrigin-RevId:
863493708)
([611f239](googleapis@611f2392))

* add dynamic channel support (#2) (googleapis#13504)
([65a3c4e](googleapis@65a3c4ec))

* add ip protocol for bigtable connection (googleapis#13520)
([6f86983](googleapis@6f86983e))

* add metrics exporter for outstandingrpcs and perconnectionerror count
(googleapis#13510)
([7f1b36a](googleapis@7f1b36ac))

* add client startup time metrics. (googleapis#13521)
([8f90da3](googleapis@8f90da38))

* introduce prime() method and load counting based on unary,streaming
and ability to remove connection (googleapis#13419)
([c6773ce](googleapis@c6773ceb))

### Bug Fixes

* handle client region and client host name based on ote… (googleapis#13752)
([0429170](googleapis@0429170f))

* fix a nil pointer deference in remove connections when… (googleapis#13535)
([9d2ba82](googleapis@9d2ba82e))

### Performance Improvements

* use passthrough with emulator endpoint (googleapis#13739)
([dafbca2](googleapis@dafbca22))

</details>
sushanb added a commit that referenced this pull request Jun 12, 2026
Fix #2: Correct routing-params headers for AuthorizedView and MaterializedView.
OpenAuthorizedView and OpenMaterializedView were emitting table_name=... as the
routing key, which is incorrect. The server routes AV traffic by
authorized_view_name and MV traffic by materialized_view_name. Add two new
helpers in bigtable.go (reqParamsHeaderValAuthorizedView and
reqParamsHeaderValMaterializedView) following the existing
reqParamsHeaderValTable pattern, and switch open.go to use them.

Fix #9: Refactor Client.Close shutdown order. Previously backgroundCancel and
configManager.Close fired before sessionMgr.Close, which meant in-flight
configManager polls could call UpdateConfig against pools that were about to
be torn down. New order: drain sessionMgr first, then stop configManager
polling, then cancel the background ctx, flush metrics, and finally close the
classic pool. Errors from each step are accumulated via errors.Join so a
failure early in the chain does not skip later cleanup.

Fix #8 (deferred): Add a TODO in NewClientWithConfig documenting the
SessionLoad pump from ClientConfigurationManager to Diverter. The existing
exported listener API (AddSessionPoolListener) only surfaces
SessionPoolConfiguration (Min/Max), not the top-level SessionLoad field, and
the underlying addListener is unexported. Wiring this up requires
ClientConfigurationManager to expose an AddSessionLoadListener (owned by
another agent), so the pump is left as a TODO rather than fabricated against
a nonexistent API.
sushanb added a commit that referenced this pull request Jun 20, 2026
…e link

#1 Session-lifetime histogram per pool
   Pool now keeps a 512-deep ring buffer of completed session lifetimes,
   appended at every removal site (CheckoutSession dead-detect,
   pruneSessions, OnClose, Pool.Close). PoolSnapshot exposes both a
   bucketed histogram (<10s / <1m / <5m / <30m / <1h / <6h / <24h / ≥24h)
   and p50/p95/p99 over the same window. sessionz renders the histogram
   as inline horizontal bars under the pool summary so a churn pattern
   (many sessions in <1m) jumps out visually.

#2 Reverse link channel → sessions
   Client.ChannelDebug now also walks the session manager and groups
   each session's log name by ChannelIndex. ChannelPoolDebug carries a
   SessionsByChannel map[int][]string populated for the "session" role
   only (classic-path RPCs have no session). channelz renders a new
   "Sessions" column listing the names of every session riding each
   channel — the inverse of the Ch# link in sessionz, so you can click
   from channelz "channel-session-2 has sessions read-1, read-3" back
   to sessionz to inspect those sessions in detail.

Pure HTML/SVG rendering, no JS, no new dependencies. Hot path
unchanged: lifetime is recorded only at retirement (one lock acquire on
a small slice), and the channel→session grouping happens inside the
ChannelDebug.Snapshot adapter only when the debug UI requests it.
sushanb added a commit that referenced this pull request Jul 10, 2026
…r invariants; rewire reviewer agents + hook

Split the monolithic SESSION_SPEC.md (was 19 invariants covering four
distinct concerns) into 4 files by concern, at repo root:

- SESSION_SPEC.md — 10 invariants, one Session's lifecycle (state
  machine, one-in-flight vRPC, PeerInfo timing, hook ordering,
  close/GOAWAY + LastRpcIdAdmitted, heartbeat, missed-heartbeat,
  retry oracle, concurrency).
- SESSION_CLIENT_SPEC.md — 4 invariants, SessionClient layer:
  Client↔SessionClient 1:1 + SessionClient↔pools 1:N + lazy pool
  creation, shared channel pool, GetClientConfiguration as
  authoritative source, OpenSessionRequest envelope with per-resource
  inner proto marshaled to Payload bytes.
- SESSION_POOL_SPEC.md — 5 invariants, pool + picking layer: read/write
  pool per resource, AFE picker discipline (NEW: two-tier flow,
  K-choice, PeakEwma OK-gated, e2eEwma-not-transportEwma per Java
  parity, PickDecision audit trail, re-entrant deadlock trap on
  p.picker.Name()), Diverter+TableShim routing, debug non-blocking,
  server-driven scaling.
- CLIENT_SIDE_METRICS_SPEC.md — 1 invariant (room to grow),
  per-attempt cluster_id/zone_id/transport-peer sourcing on classic
  vs session paths.

SESSION_POOL_SPEC.md #2 (picker) is new content — sourced from
CLAUDE.md and memory files; previously undocumented in-spec.

SESSION_COMPONENT_SPEC.md unchanged in content; intro updated to name
the 4 sibling behavioral specs.

Reviewer agents:
- session-reviewer: description + workflow rewritten to read all 4
  spec files; report table gains a Spec column ({SESSION, CLIENT,
  POOL, METRICS}) so citations disambiguate across files.
- session-component-review: intro sentence updated to reference the
  4 behavioral specs by name.

PostToolUse hook (.claude/settings.json):
- Regex extended to match SESSION_CLIENT_SPEC.md, SESSION_POOL_SPEC.md,
  CLIENT_SIDE_METRICS_SPEC.md, plus bigtable/debugview/, afe_picker.go,
  and diverter.go.
- Stderr message rewritten to explicitly instruct invoking both
  session-reviewer and session-component-review agents in parallel
  before committing this batch of edits.

Verified: 10 + 4 + 5 + 1 = 20 invariants (every original one has a
new home), no dangling old-numbering cross-references, all cross-file
refs use FILE.md #N form, hook regex accepts all intended paths and
rejects unrelated ones.
sushanb added a commit that referenced this pull request Jul 10, 2026
…itted, formalize retry-signal provenance

CLIENT_SIDE_METRICS_SPEC.md
- New invariant #2: per-attempt latency labels on the session path (client_blocking_latencies from SentAt, server_latencies from Stats.BackendLatency, transport_type/*) MUST be sourced only from InvokeResult and stamped in exactly one place — sessionTable.stampAttempt. Corrects a prior wrong claim that InvokeResult.TransportLatency was not a client-side metric.
- New invariant #3: session-tracer-hosted OTel histograms (session.durations, session.open_latencies, session.uptime, transport_latencies) — registered once by InitializeSessionMetrics, emitted only by sessionTracer, session_name label is pool-scoped (bounded) not Session.LogName (unbounded).
- Scope line updated to reference session_tracer.go and session_pool.go recording sites; invariant count bumped to 3.

SESSION_SPEC.md
- Invariant #6: deprecate the LastRpcIdAdmitted retry oracle. Proto field is still sent by the server but the Go transport does not read it (git grep returns zero hits in internal/transport/). Section heading trimmed; deferred-work callout notes any reintroduction requires paired code+spec+test change with a Java-parity check.
- Invariant #9: new 'Retry-signal provenance' table splitting server-only inputs (RetryInfo, narrow always-retry code set, x-goog-cbt-cookie-routing-cookie) from client-only inputs (AttemptState tag, idempotency check, 3-attempt cap, deadline-fit, zero-value fail-closed). Absorbs several redundant bullets from the prior 'Additional invariants' list.
- Added 'How to verify' pointer to CLAUDE.md smoke-gate for consistency (2 of 5 specs updated; remaining specs to follow).

SESSION_CLIENT_SPEC.md
- Added 'How to verify' pointer.

SESSION_COMPONENT_SPEC.md
- New Part C row for session-tracer OTel histograms: sessionTracer sole writer, registered once by InitializeSessionMetrics, transport_latencies recorded only from session_pool.go:650 under the positive-delta gate, session_name must be pool-scoped not LogName.
sushanb added a commit that referenced this pull request Jul 10, 2026
…hook ordering)

Adds two tests to session_test.go closing coverage gaps flagged by the
spec test-coverage audit:

TestInvoke_ConcurrentSecondFailsWithMultiplexViolation (SESSION_SPEC #2)
- Puts session in StateReady via makeActive, pins activeRPC.Store with a
  placeholder vrpcImpl, then calls Invoke. Asserts (a) error wraps
  ErrSessionNotActive, (b) ClassifyErr(err).State == StateUncommitted
  (retryable — attempt never hit wire), (c) error message contains
  'multiPlexingLimit=1' for operator grep.
- Exercises the previously-unreached CAS-fail branch at
  session_vrpc.go:107-109. Prior tests only asserted the multiPlexingLimit
  constant value, never the CAS-fail runtime path.

TestHooks_FiredInSpecOrder (SESSION_SPEC #4)
- Records OnStart / OnActive / OnClosing / OnClose invocations into a
  mutex-guarded []string, drives Start -> OpenSession response ->
  ForceClose, asserts exact order ['OnStart', 'OnActive', 'OnClosing',
  'OnClose'] via reflect.DeepEqual.
- Prior hookCounts fixture recorded only 3 of 4 hooks (no OnClosing) and
  only checked counts, so a regression that swapped OnClosing and OnClose
  would slip through. ForceClose exercises both the explicit
  notifyClosing call at session_lifecycle.go:77 and the safety-net call
  inside notifyClosed at :106.

Both tests pass; full internal/transport suite passes.
sushanb added a commit that referenced this pull request Jul 10, 2026
…ant rows, dedicated reviewer slide

Slide 2 — invariant table:
- Trimmed SESSION_SPEC #6 example to just the Ready→Closing state transition (drop the in-flight-vRPC / 30s grace details that belonged in the full spec, not the deck cell).
- Rewrote SESSION_POOL_SPEC #2 to name both production picker strategies (LeastInFlight for fewest active vRPCs, LeastLatency using e2eEwma with Java parity) instead of only calling out the e2eEwma vs transportEwma distinction.
- Strengthened SESSION_COMPONENT_SPEC B2 example to explain layer direction + rationale + why it is compiler-enforced (Go import-cycle detector) with the grep as belt-and-braces, not the primary check.
- Removed the 'concrete failure mode' callout that duplicated the same GOAWAY example.

Slide 3 — Part C excerpt: dropped the In-flight vRPC slot row (already implied by the surrounding spec discussion, and the row was redundant given the SESSION_SPEC #2 coverage on slide 2).

Slide 4 — big-feature example replaced:
- Was: unified debugview/ package fold.
- Now: tcpz page (per-connection TCP_INFO for every conn-pool connection). Two-sentence prompt + why the specs made it survivable (B3 forced a fourth Handler arg for TCPStats, POOL #4 kept getsockopt syscalls outside pool locks, B10 kept the sub-1500 PMTU flag on the DTO producer). Six-commit iteration story preserved.

Slide 5 — new dedicated slide 'How the reviewer verifies specs and runs tests':
- Extracted the reviewer flow that used to sit at the bottom of slide 4.
- Adds report-format example table (PASS / VIOLATION / AMBIGUOUS with example rows).
- Adds the smoke-gate command: go test ./internal/transport/ -count=1 -short -timeout=90s.
- Mentions the two new spec-enforcing tests (TestInvoke_ConcurrentSecondFailsWithMultiplexViolation, TestHooks_FiredInSpecOrder) as concrete coverage examples.

Slide counter updated 4 → 5; nav shortcuts (1–5) still work.
sushanb added a commit that referenced this pull request Jul 17, 2026
…ines ordering

Two Java-parity + SESSION_SPEC fixes surfaced by the 2026-07-17 review
pass on feat/bigtable-sessionz-debug.

L5 — PeakEwma seed constants were defined but never applied. afeHandle
constructed its transportEwma and e2eEwma via NewPeakEwma(afePeakEwmaTau),
so new AFEs returned Value() = 0. LeastLatencyAfePicker's min-cost scan
pinned traffic to the newest AFE until it accumulated ≥1 OK sample,
violating SESSION_POOL_SPEC #2's Java-parity clause "New AFEs don't win
by looking free-cost."

- peak_emwa.go: add NewPeakEwmaSeeded(tau, seed). Sets value = float64(seed)
  and leaves lastUpdate zero so Update's IsZero branch replaces the seed
  with the first real sample — matches Java's SessionList seed-until-
  first-sample semantics.
- session_list.go: wire afeTransportEwmaSeed (500µs) / afeE2eEwmaSeed (1ms)
  at afeHandle construction.
- session_list_test.go, session_pool_afe_test.go: update
  RecordVRpcOutcome_SkipsNonOK and Pool_LeastLatency_IgnoresFailingAFELatency
  to assert Value() == afeE2eEwmaSeed (seed unchanged) instead of == 0
  after non-OK records. The OK-gate invariant they check is the same;
  only the untouched-value baseline moved from 0 to the seed.

L2 — OnStart hook fired AFTER go readLoop / go heartBeatLoop spawned in
Start(). On a fast handshake, readLoop could Recv OpenSessionResponse and
fire onActive before onStart returned — SESSION_SPEC #4 hook-order
violation (onStart → onActive → onClosing → onClose). SessionPoolImpl.OnStart
is a no-op today so no observable impact, but the fix defends future
non-no-op OnStart consumers by construction.

- session_lifecycle.go: move s.hooks.onStart(ctx) to before the readLoop
  and heartBeatLoop spawn in Start(). OnStart takes ctx and has no
  dependency on readLoop, so the reorder is safe.

Passes go test ./internal/session/ ./internal/transport/ ./debugview/
-count=1 -race -short. Behavioral + boundary spec reviews both PASS.
sushanb added a commit that referenced this pull request Jul 18, 2026
v1 (854d216) ported Java-parity slotMu one-in-flight lifecycle:
ctx.Done leaves the slot claimed, drainSlot on the server response
frees it. But the pool's Invoke defer still called releaseSession +
signalFree unconditionally, so a session with an undrained slot could
land back in its AFE idle queue while the slot was still busy. v2
(e3fb88a) reconciled with a busy-skip loop in sessionList.Checkout.
Two mechanisms racing to describe "session is free"; the busy-skip
loop is the reconciliation cost.

v3 collapses them. The AFE idle queue contains only sessions with
empty slots, by construction. Only drainSlot success re-enqueues and
wakes; the pool's Invoke return path no longer touches the queue or
the wake channel. Checkout goes back to a plain dequeue.

Change list:

- session_pool.go Invoke defer — drops releaseSession(sh) + signalFree().
  Keeps DecOutstanding + recordVRpcOutcome (per-caller in-flight
  counter + per-AFE PeakEwma update; those piggyback on the caller's
  return, not on drain, because the caller is where invokeErr /
  latency / backendDur are known).

- session_vrpc.go — notifySlotDrained now fires from every request-
  path drainSlot success: normal handleVRPCResponse (was cancelled-
  branch-only), normal handleVRPCErrorResponse (same), and the Send-
  failure branch of Invoke. cancelActiveRPCs (session teardown) still
  does NOT fire the callback — OnSessionClosing/OnSessionClosed
  handle removal from routing structures.

- session_pool_lifecycle.go OnActive — onSlotDrained callback grows
  from "signalFree only" to "releaseSession(sh) + signalFree()".

- session_list.go Checkout — the v2 busy-skip loop deletes; plain
  dequeue restores. Docstring pins the new invariant ("dequeued
  handles are guaranteed idle-slot by construction — drainSlot is
  the sole re-enqueue site").

Ordering caveat (documented, accepted): the wake fires from the
response-handler goroutine BEFORE Invoke's defer runs, so a picker on
another goroutine may see pre-update EWMAs for one tick. Same window
v2 already ships on the cancelled-drain path; v3 generalizes it.

Complexity delta:

- 25 lines gone from Checkout (busy-skip loop + snapshotted-length
  reasoning + tail-rotation + ready-set gymnastics).
- 66 lines gone from session_list_test.go (v2's
  TestPool_CheckoutSkipsSessionWithUndrainedSlot — the invariant it
  pinned is now unreachable by construction).
- 3 net lines added in session_vrpc.go (notifySlotDrained calls in
  two normal branches + Send-failure branch).
- Dual-authority invariant collapses to a single owner: drainSlot.
- The v1/v2 window where Invoke's defer re-enqueues a busy session
  and the next Checkout has to skip it never happens.

Specs paired:

- SESSION_SPEC.md #2 — "Every request-path drainSlot success fires
  notifySlotDrained" (enumerates the five fire sites; exempts
  cancelActiveRPCs as a session-teardown path).
- SESSION_POOL_SPEC.md #2 — "wake-ups fire centrally from Invoke's
  defer" rewritten to "from Session.notifySlotDrained".
- SESSION_POOL_SPEC.md #6 — state-machine transition table grew a
  "Triggered by" column; InFlight→Idle row cites the drain callback.
- SESSION_COMPONENT_SPEC.md — new ownership row for the "Session→pool
  slot drained signal" naming Session.notifySlotDrained →
  SessionHandle.onSlotDrained as the sole path.

Green: transport + debugview -race -short (12.8s + 1.0s);
TestInvoke_ForceCloseRace_BoundedReturnUnderCtx ×200 -race (58.9s);
three new v3 tests pinning the drain-driven wake — normal
handleVRPCResponse deliver, normal handleVRPCErrorResponse deliver,
Send-failure Invoke branch — all PASS under -race. session-reviewer
and session-component-review both PASS with no violations. Pre-
existing top-level bigtable integration failures (TestIntegration_
SessionVRpc_* Diverter-flip timeouts) reproduce on the pre-v3
baseline — unrelated to this change.

Plan: ~/.claude/plans/session-slotmu-v3.md.
sushanb added a commit that referenced this pull request Jul 20, 2026
Client releases the Session slot in Invoke's defer today, so a caller
ctx.Done frees it and the next Invoke immediately claims a fresh
rpc_id — the late server response for the abandoned rpc lands with an
id-mismatch drop. Java holds currentRpc until the server response
arrives (SessionImpl.java:448-457, 599-602) and rejects a concurrent
startRpc with UNCOMMITTED so the retryer steers away. This experiment
ports that discipline.

Session.activeRPC becomes a plain *vrpcImpl paired with a new
currentCancel under slotMu — the Go analog of Java's
sessionSyncContext. Three intent-named ops replace the atomic CAS
wrapper: claimSlot (Invoke), markCancelled (ctx.Done), drainSlot
(response handlers + cancelActiveRPCs). Invoke's defer releaseSlot is
gone; the slot lives until the server response drains it or the
session tears down. handleVRPCResponse / handleVRPCErrorResponse
gate id-match before drainSlot; if currentCancel is set they record
tagSessionVRPCCancelledDrained (new) and skip deliver — nobody's
waiting on resultChan. A losing claimSlot returns Uncommitted named
"session busy: prior vRPC has not drained on the wire" (Java
createUncommitedError parity — retry allowed).

Falls out cheap: deliver collapses to a plain send (drainSlot makes
the two-writers race impossible); tagSessionVRPCDuplicateResult
retires; the internal-bug id-mismatch branch in awaitInvokeResult
becomes unreachable and deletes; the "order matters: clear slot,
THEN check state" gymnastics around releaseSlot disappear. Send
failure still drains explicitly (Go Send is synchronous where Java's
sendMessage is async). Retry classification of ctx.Done stays
StateTransportFailure for v1 — Java's `rejected` bit is deferred
follow-up (plan file: session-java-parity-slot).

Tests: TestHandleVRPCResponse_DuplicateFrame_FlagsDuplicate and
TestDeliver_CancelRacingCompletion_FlagsEvent delete outright
(unreachable). TestHandleVRPCResponse_LateResponseAfterCtxDone_FlagsNilSlot
inverts into _FlagsCancelledDrained (slot HELD after ctx.Done; tag
fires on drain). Adds TestInvoke_SecondInvokeAfterCtxDoneRejectedUncommitted
pinning Java-parity claim rejection. 15 activeRPC.Store fixtures
move to setSlotForTest.

Green: -race -short (transport + debugview), TestInvoke_ForceClose
x200 -race, retry-oracle full suite. Full plan and file-by-file
change list at ~/.claude/plans/prancy-zooming-waffle.md.

Spec paired: SESSION_SPEC #2 (claim/drain lifecycle) + #10 (slotMu
as innermost second mutex; deliver single-writer); COMPONENT_SPEC
ownership row updated to (activeRPC, currentCancel) under slotMu.
sushanb added a commit that referenced this pull request Jul 20, 2026
v1 (854d216) ported Java-parity slotMu one-in-flight lifecycle:
ctx.Done leaves the slot claimed, drainSlot on the server response
frees it. But the pool's Invoke defer still called releaseSession +
signalFree unconditionally, so a session with an undrained slot could
land back in its AFE idle queue while the slot was still busy. v2
(e3fb88a) reconciled with a busy-skip loop in sessionList.Checkout.
Two mechanisms racing to describe "session is free"; the busy-skip
loop is the reconciliation cost.

v3 collapses them. The AFE idle queue contains only sessions with
empty slots, by construction. Only drainSlot success re-enqueues and
wakes; the pool's Invoke return path no longer touches the queue or
the wake channel. Checkout goes back to a plain dequeue.

Change list:

- session_pool.go Invoke defer — drops releaseSession(sh) + signalFree().
  Keeps DecOutstanding + recordVRpcOutcome (per-caller in-flight
  counter + per-AFE PeakEwma update; those piggyback on the caller's
  return, not on drain, because the caller is where invokeErr /
  latency / backendDur are known).

- session_vrpc.go — notifySlotDrained now fires from every request-
  path drainSlot success: normal handleVRPCResponse (was cancelled-
  branch-only), normal handleVRPCErrorResponse (same), and the Send-
  failure branch of Invoke. cancelActiveRPCs (session teardown) still
  does NOT fire the callback — OnSessionClosing/OnSessionClosed
  handle removal from routing structures.

- session_pool_lifecycle.go OnActive — onSlotDrained callback grows
  from "signalFree only" to "releaseSession(sh) + signalFree()".

- session_list.go Checkout — the v2 busy-skip loop deletes; plain
  dequeue restores. Docstring pins the new invariant ("dequeued
  handles are guaranteed idle-slot by construction — drainSlot is
  the sole re-enqueue site").

Ordering caveat (documented, accepted): the wake fires from the
response-handler goroutine BEFORE Invoke's defer runs, so a picker on
another goroutine may see pre-update EWMAs for one tick. Same window
v2 already ships on the cancelled-drain path; v3 generalizes it.

Complexity delta:

- 25 lines gone from Checkout (busy-skip loop + snapshotted-length
  reasoning + tail-rotation + ready-set gymnastics).
- 66 lines gone from session_list_test.go (v2's
  TestPool_CheckoutSkipsSessionWithUndrainedSlot — the invariant it
  pinned is now unreachable by construction).
- 3 net lines added in session_vrpc.go (notifySlotDrained calls in
  two normal branches + Send-failure branch).
- Dual-authority invariant collapses to a single owner: drainSlot.
- The v1/v2 window where Invoke's defer re-enqueues a busy session
  and the next Checkout has to skip it never happens.

Specs paired:

- SESSION_SPEC.md #2 — "Every request-path drainSlot success fires
  notifySlotDrained" (enumerates the five fire sites; exempts
  cancelActiveRPCs as a session-teardown path).
- SESSION_POOL_SPEC.md #2 — "wake-ups fire centrally from Invoke's
  defer" rewritten to "from Session.notifySlotDrained".
- SESSION_POOL_SPEC.md #6 — state-machine transition table grew a
  "Triggered by" column; InFlight→Idle row cites the drain callback.
- SESSION_COMPONENT_SPEC.md — new ownership row for the "Session→pool
  slot drained signal" naming Session.notifySlotDrained →
  SessionHandle.onSlotDrained as the sole path.

Green: transport + debugview -race -short (12.8s + 1.0s);
TestInvoke_ForceCloseRace_BoundedReturnUnderCtx ×200 -race (58.9s);
three new v3 tests pinning the drain-driven wake — normal
handleVRPCResponse deliver, normal handleVRPCErrorResponse deliver,
Send-failure Invoke branch — all PASS under -race. session-reviewer
and session-component-review both PASS with no violations. Pre-
existing top-level bigtable integration failures (TestIntegration_
SessionVRpc_* Diverter-flip timeouts) reproduce on the pre-v3
baseline — unrelated to this change.

Plan: ~/.claude/plans/session-slotmu-v3.md.
sushanb added a commit that referenced this pull request Jul 22, 2026
…lers

handleVRPCResponse and handleVRPCErrorResponse were 90% identical —
same state gate, activeVRPC nil check, id-match guard, drainSlot,
counter bump, cancel-vs-deliver branch, notifySlotDrained, and Closing
signalQuiescent, differing only in the frame proto, nil-tag, counter,
and vrpcResult payload. Fold the shared skeleton into a private
routeVRPCFrame(rpcID, frameName, nilTag, counter, result) so the two
call sites can't drift on any of the Java-parity gating.

Log wording change: the wrongState assertion + drop debugf now use the
proto frame name uniformly ("VirtualRpcResponse" / "ErrorResponse")
instead of the split "vRPC response"/"vRPC error" (wrongState) vs
proto name (debugf) pair. No callers grep the text.

Behavioral parity verified by session-reviewer (SESSION_SPEC #2/#5/#6/
#10) and session-component-review (Part B B2/B7/B11 + Part C).
sushanb added a commit that referenced this pull request Jul 22, 2026
Deletes the bare Session.slotDrainedFn field and its setSlotDrainedCallback
setter / notifySlotDrained fire method in favor of a fifth field on the
existing SessionHooks struct — OnSlotDrained. The struct already carries
the four ordered lifecycle callbacks (OnStart / OnActive / OnClosing /
OnClose) wired at construction; the drain callback was the only reason
Session carried a second, parallel hook mechanism with its own nil-guard
and its own doc block.

After: one hook mechanism on Session. Every fire site still fires on the
same drain paths (Send-failure early drain, response-frame drain,
error-frame drain via routeVRPCFrame); cancelActiveRPCs still does NOT
fire it. The pool's installer folds into the same SessionHooks{...}
literal createSession was already building.

Also refreshes SESSION_SPEC #2, #10, SESSION_POOL_SPEC #2, #6, and
SESSION_COMPONENT_SPEC B7 + Part C to name the new identifiers instead
of slotDrainedFn / notifySlotDrained / setSlotDrainedCallback.

session-reviewer (4 behavioral specs) and session-component-review
(SESSION_COMPONENT_SPEC B1-B12 + Part C) returned PASS on the diff.
sushanb added a commit that referenced this pull request Jul 22, 2026
…construction

Java-parity port of SessionPoolImpl.java:424-448. Deletes the
Session→SessionHandle back-ref (Session.poolHandle) whose nil-ness
was abused as a "already handled" signal, and wires per-session
SessionHooks closures at createSession time that capture *SessionHandle
directly. The pool never walks back through Session to find the handle.

Shape changes:
- SessionHandle gains three atomic.Bool one-shot dedup flags:
  activated (defensive onActive re-entry gate) and
  closingRecorded / closeRecorded (Pool.Close Phase1↔Phase2 short-
  circuit; NOT a substitute for SESSION_SPEC #4 exactly-once).
  Loses the onSlotDrained field (moves onto Session).
- Session loses poolHandle atomic.Pointer[SessionHandle]. Gains
  slotDrainedFn func() with a lowercase setSlotDrainedCallback setter.
  notifySlotDrained now calls the field directly (no atomic Load, no
  pointer chase). Single-writer-at-construction: setter called from
  createSession before Session.Start spawns any goroutine.
- SessionPoolImpl.startingSessions retyped from map[*Session]bool to
  map[*SessionHandle]struct{}.
- SessionPoolImpl.createSession mints the handle before NewSession,
  builds hooks closures capturing sh, installs the slot-drained
  closure via setSlotDrainedCallback, backfills sh.session /
  sh.createdAt in two statements immediately after.
- SessionPoolImpl.{OnActive,OnClosing,OnClose} renamed to lowercase
  onActive/onClosing/onClose taking *SessionHandle. onActive CAS-
  gates on sh.activated. onClosing / onClose CAS-gate on the paired
  dedup flag; short-circuit when Pool.Close Phase-1 already ran the
  bookkeeping.
- Pool.Close Phase-1 flips both dedup flags on each snapshot handle
  in order: recordLifetime → recordSessionClose → flip flags →
  sl.OnSessionClosed. Replaces the prior sh.session.poolHandle.Store(nil)
  trick, expressing the dedup as an actual dedup flag instead of
  abusing a back-ref's nullability.

Test migration:
- injectActiveSession / injectActiveOnAfe rewired to the new closure-
  hooks shape. The six test sites that used to do s.poolHandle.Store(sh)
  by hand become no-ops (or move to s.setSlotDrainedCallback(...) for
  the drain-signal tests in session_vrpc_test.go).
- session_pool_lifecycle_test callers use lowercase p.onActive(sh) /
  p.onClosing(sh) / p.onClose(sh, err). The poolHandle assertions
  in TestOnClosing_DropsFromReadyCountAndRecordsLifetime and
  TestOnClosing_StartingSessionIsNoOp were replaced (the former
  drops the check outright; the latter now asserts sh.closingRecorded
  stays unset for a starting-only handle).

Spec updates (paired with code, both reviewers green):
- SESSION_SPEC.md #2 / #10: slot-drained wire re-described; poolHandle
  dropped from the atomic-state list; slotDrainedFn documented as
  single-writer-at-construction.
- SESSION_POOL_SPEC.md #6 transition table (NotRegistered→Idle and
  InFlight→Idle rows) point at Session.slotDrainedFn and lowercase
  SessionPoolImpl.onActive.
- SESSION_COMPONENT_SPEC.md B7 whitelist swaps poolHandle for
  slotDrainedFn; Part C ownership matrix updates the slot-drained
  row (install site now createSession, callback location now Session);
  new Part C row for the three atomic.Bool flags with explicit scope
  disclaimer.

Behavior shift worth flagging (not a spec violation):
- The "pool closed before session became active" race in onActive
  now runs onClosing's recordLifetime path once (createdAt is set
  at createSession, so time.Since is meaningful). Old back-ref
  path skipped it because poolHandle was never stored. Adds one
  lifetimes-ring entry per race case; no spec pins lifetimes-ring
  semantics for this race so treated as improvement, not regression.

Pre-existing race unchanged by this diff:
- TestHeartBeatLoop_ForceClosesOnMissedHeartbeat leaks a heartBeatLoop
  goroutine that races with TestSessionTracer_MetricsRoundTrip's
  global metric init. Reproduces on the base branch under -race
  -count>1; not caused by this refactor.
sushanb added a commit that referenced this pull request Jul 22, 2026
Deletes the bare Session.slotDrainedFn field and its setSlotDrainedCallback
setter / notifySlotDrained fire method in favor of a fifth field on the
existing SessionHooks struct — OnSlotDrained. The struct already carries
the four ordered lifecycle callbacks (OnStart / OnActive / OnClosing /
OnClose) wired at construction; the drain callback was the only reason
Session carried a second, parallel hook mechanism with its own nil-guard
and its own doc block.

After: one hook mechanism on Session. Every fire site still fires on the
same drain paths (Send-failure early drain, response-frame drain,
error-frame drain via routeVRPCFrame); cancelActiveRPCs still does NOT
fire it. The pool's installer folds into the same SessionHooks{...}
literal createSession was already building.

Also refreshes SESSION_SPEC #2, #10, SESSION_POOL_SPEC #2, #6, and
SESSION_COMPONENT_SPEC B7 + Part C to name the new identifiers instead
of slotDrainedFn / notifySlotDrained / setSlotDrainedCallback.

session-reviewer (4 behavioral specs) and session-component-review
(SESSION_COMPONENT_SPEC B1-B12 + Part C) returned PASS on the diff.
sushanb added a commit that referenced this pull request Jul 24, 2026
…pollingLoop

pollingLoop's inner m.pollsWG.Add(1) before poll() (line ~465) could
fire after Close's m.pollsWG.Wait() had started, panicking with
"sync: WaitGroup misuse: Add called concurrently with Wait" when the
counter was momentarily at 0. Compounded by pollingLoop's own
goroutine not being tracked in pollsWG at all — Close's Wait never
awaited the loop itself.

Self-track pollingLoop in pollsWG: Add(1) before `go m.pollingLoop()`
in Start, defer Done() at the top of the loop. This keeps the counter
≥ 1 for the loop's lifetime, so the inner per-poll Add(1) can never
race Wait's counter-hits-0 wake. Also guarantees Close waits for the
loop itself to exit, not just its child polls.

Found by the golang-concurrency skill audit (MED #2 + MED #3 folded
into one fix per the audit's own recommendation).
sushanb added a commit that referenced this pull request Jul 25, 2026
Add two observation-only debug tags fired from stampAttempt when the
session-path attempt's InvokeResult lacks a real ClusterInformation:

  - TagSessionAttemptNilClusterInfo — ClusterInfo is nil (transport
    failure with no server response, OR server response omitted
    ClusterInformation entirely).
  - TagSessionAttemptEmptyClusterID — ClusterInformation is present
    but ClusterId is empty (server contract violation per
    CLIENT_SIDE_METRICS_SPEC #1).

Both are the input conditions for the reported symptom: session-path
attempts get labeled cluster_id=<unspecified> in downstream metrics
because stampAttempt has nothing to stamp AND session path passes
nil header/trailer MDs to RecordAttemptCompletion, so
ExtractLocation returns the sentinel defaultCluster. Dashboards that
filter attempt_latencies2 on real cluster then hide the affected
attempts while connectivity_error_count (labeled <unspecified>)
still surfaces them, producing the "no errors in latencies but
UNAVAILABLE on connectivity" mismatch.

Land as observation-only first — confirm frequency correlates with
the reported incidents before landing any labeling-fix (candidates:
propagate currOp.lastClusterID from stampAttempt, or broaden the
connectivity-error gate to trust transportType populated).

Also introduces a typed DebugTag string alias + exports RecordDebugTag
so packages under bigtable/internal can fire from their own layer
without drifting off the tag catalog on a raw string. Session→transport
is the allowed direction per Part B; debug-tag registry ownership
stays with the transport package.

Reviewers: session-reviewer PASS on METRICS #1/#2 + POOL #4;
session-component-review PASS on B1/B2/B6/B9 + Part C ClusterInfo
ownership row; igor asked for typed DebugTag which is applied.
sushanb added a commit that referenced this pull request Jul 27, 2026


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).
sushanb added a commit that referenced this pull request Jul 27, 2026
… source

Move the transport-overhead subtraction from the pool layer into the
Session layer so the semantic is canonical at the producer, not
re-derived by each consumer.

Changes:
* InvokeResult.WireLatency (raw round-trip Send→Recv) added; renamed
  to E2ELatency mid-review per igor-reviewer's Java-parity ask
  (SessionList.updateLatency in Java takes (e2eLatency, backendLatency),
  and the picker docstring already leans on "e2eEwma" terminology).
* InvokeResult.TransportLatency now = E2ELatency − BackendLatency
  (guarded > 0 for clock-skew protection). Zero when Stats is missing,
  the call errored pre-Recv, or the subtraction was non-positive.
* Session.processResult is the sole computation site.
* SessionPoolImpl.Invoke drops the local subtraction; consumes the
  pre-computed value directly. Cleaner hot path + eliminates the
  duplication risk (only one place doing the subtraction).
* SlowVRpcEvent gains an E2ELatency field so the slow-vRPC forensics
  view keeps the raw round-trip alongside the overhead-only value
  (per igor-reviewer nit — losing the raw round-trip on the slow row
  is a step backwards for post-hoc debugging).

Tests:
* New TestInvoke_E2EAndTransportLatency (three subtests):
  - nil Stats → E2ELatency > 0, TransportLatency = 0.
  - backend > wire (clock skew) → E2ELatency > 0, TransportLatency = 0.
  - backend < wire → TransportLatency = E2ELatency − backend.
  Locks the semantic swap against future refactors.

Spec updates (paired with the code move):
* CLIENT_SIDE_METRICS_SPEC.md #2: TransportLatency description now
  reflects E2ELatency + processResult as the computation site.
* CLIENT_SIDE_METRICS_SPEC.md #3: transport_latencies sole-writer
  contract now references the source-side gate.
* SESSION_COMPONENT_SPEC.md Part C transport_latencies row: pointer
  to the positive-delta gate now points at session_vrpc.go
  processResult (was session_pool.go:646-647).
sushanb added a commit that referenced this pull request Jul 28, 2026
…go + client.go

- MaxAttempts 10 → 3 in session-table retry interceptor (Java parity —
  RetryingVRpc.java:309). Test cap updated to match; dropped stale
  TODO block that flagged the divergence as a follow-up.
- Dropped 2 observation-only RecordDebugTag calls in stampAttempt
  (TagSessionAttemptNilClusterInfo, TagSessionAttemptEmptyClusterID) —
  the diagnosis they were meant to confirm has landed and the tags
  are no longer wanted.
- Comment on SetClientBlockingLatency explaining the semantic:
  SentAt - AttemptStartTime = elapsed time from attempt-start to when
  the request was Sent on the bidi stream.
- New sessionProtocolVersion = 1 const used in OpenSessionRequest
  envelope (replaces the literal). Bump on non-backwards-compatible
  wire-shape changes.

Addresses inline review comments on PR googleapis#20228 at table.go:191/195/267/
269/279 and client.go:595. Fifth outstanding nit (session/table.go:283
SetServerLatency=0) held for clarification — reviewers flagged it as
a CLIENT_SIDE_METRICS_SPEC #2 violation.
sushanb added a commit that referenced this pull request Jul 28, 2026
…go + client.go

- MaxAttempts 10 → 3 in session-table retry interceptor (Java parity —
  RetryingVRpc.java:309). Test cap updated to match; dropped stale
  TODO block that flagged the divergence as a follow-up.
- Dropped 2 observation-only RecordDebugTag calls in stampAttempt
  (TagSessionAttemptNilClusterInfo, TagSessionAttemptEmptyClusterID) —
  the diagnosis they were meant to confirm has landed and the tags
  are no longer wanted.
- Comment on SetClientBlockingLatency explaining the semantic:
  SentAt - AttemptStartTime = elapsed time from attempt-start to when
  the request was Sent on the bidi stream.
- New sessionProtocolVersion = 1 const used in OpenSessionRequest
  envelope (replaces the literal). Bump on non-backwards-compatible
  wire-shape changes.

Addresses inline review comments on PR googleapis#20228 at table.go:191/195/267/
269/279 and client.go:595. Fifth outstanding nit (session/table.go:283
SetServerLatency=0) held for clarification — reviewers flagged it as
a CLIENT_SIDE_METRICS_SPEC #2 violation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants