fix(bigtable): guard NewStream OnFinish against grpc-go double-fire - #20295
Merged
sushanb merged 3 commits intoAug 3, 2026
Conversation
grpc-go v1.82.1 can invoke a registered grpc.OnFinish callback twice on some stream-creation failure paths — cs.finish → endOfClientStream fires it once from inside withRetry, then newClientStream's deferred endOfClientStream fires it again on the way out. On top of that, the old code manually decremented streamingLoad on the returned-error path (assuming OnFinish had NOT fired), so streamingLoad ended up going 2-3 decrements deep per failed NewStream. The channelz debug page surfaced this as "Streaming in flight: -9" on an idle classic pool. Trust OnFinish as the single source of truth for load accounting and error attribution, and gate the callback body with atomic.Bool CAS so it decrements exactly once per NewStream regardless of how many times grpc-go fires it. Adds NewStreamImmediateFailureLoadStaysNonNegative — closes the pool's underlying ClientConn, then loops NewStream 5 times and asserts streamingLoad never dips below 0 and ends at 0.
…name Addresses round-1 reviews from three reviewers on a129f06. Belt-and-suspenders fallback for the zero-fire path (thermo, sushanb, igor — all three flagged the same hazard). Extract the callback body into a local `finish := func(err error) {...}` closure, register it via `grpc.OnFinish(finish)`, AND after `entry.conn.NewStream` returns an error, call `finish(err)` directly. The CAS gate ensures at-most- once accounting regardless of who calls first: if grpc-go already fired OnFinish (once or twice), the CAS is lost and the fallback is a no-op; if grpc-go returned an error before arming OnFinish (no current path bigtable exercises does this, but the invariant is now load-bearing and OnFinish is a best-effort callback), the fallback picks it up. Same result — one decrement per NewStream that received a slot from streamingLoad.Add(1). Rename `finished` → `onFinishFired` (thermo + sushanb). "finished" reads as stream-state ("the stream is finished") not callback-state ("the accounting closure has run"), which is what the flag actually tracks. The rename removes the ambiguity now that two call sites (OnFinish + error-branch fallback) race for the CAS. Doc rewrite (igor + thermo). Reworded to explicitly say "AT MOST ONCE ... regardless of how many times grpc-go invokes OnFinish (0, 1, or more)", enumerating the three paths (nominal, double-fire, zero-fire). Kept the load-bearing "-9" symptom that pins the concrete regression. Dropped the grpc-go internal symbol names (withRetry, cs.finish, endOfClientStream) that would rot on any upstream rename. Test cleanup: - Rename subtest NewStreamImmediateFailureLoadStaysNonNegative → ImmediateFailureLoadStaysNonNegative (igor — parent already names TestPoolNewStream). - Drop stream.CloseSend() in the shouldn't-succeed branch (igor — t.Fatalf makes it moot). - Add test comment noting the "== 0" final assert also silently pins "accounting fired at least once per iter" — if a future grpc-go regressed to zero-firing, this reads as == 5, not < 0 (sushanb). New test TestPoolNewStream/OnFinishFiresAtLeastOncePerFailedNewStream (sushanb + igor). Wraps grpc.OnFinish with a counting helper to observe raw fires per NewStream on a closed-conn path. Asserts each attempt's fire count is small (typically 1 or 2), and that fires == 0 is not fatal (it just means the belt-and-suspenders fallback carried the accounting). Regardless of grpc-go's distribution of fires, load must land at 0. Pins the "at least once" invariant the whole fix leans on — separate from the accounting invariant.
Contributor
There was a problem hiding this comment.
Code Review
This pull request addresses an issue where streamingLoad could drift negative due to grpc-go's OnFinish callback firing multiple times on stream-creation failures. It introduces an atomic.Bool CAS gate (onFinishFired) inside a finish closure to ensure load accounting and error attribution run at most once per NewStream call. Additionally, it adds a fallback mechanism to handle cases where OnFinish is never triggered, and includes regression tests to verify the fix. I have no further feedback to provide as there are no review comments.
Follow-up to igor round-2 on PR googleapis#20295. Delete TestPoolNewStream/OnFinishFiresAtLeastOncePerFailedNewStream — its name promised an invariant it did not actually enforce (the zero- fire case was t.Logf, not t.Errorf), its n > 3 assert tested grpc-go's internal fanout rather than any behavior of ours, and its 5 ms time.Sleep for OnFinish flush was a CI flake risk under load. The streamingLoad == 0 tail assertion duplicated what ImmediateFailureLoadStaysNonNegative already covers. Also trim the overstated "== 0 also silently pins the fire" comment in ImmediateFailureLoadStaysNonNegative — the assertion pins load landing at 0, which is what matters; whether OnFinish fired once and was CAS-gated or fired zero times and the fallback carried it is not something the assertion actually distinguishes. The load-bearing regression coverage stays on TestPoolNewStream/ImmediateFailureLoadStaysNonNegative, which is what actually pins the fix.
mutianf
approved these changes
Aug 3, 2026
sushanb
pushed a commit
that referenced
this pull request
Aug 3, 2026
🤖 I have created a release *beep* *boop* --- ## [1.52.0](bigtable/v1.51.0...bigtable/v1.52.0) (2026-08-03) ### Features * **bigtable:** Add AFE picker (Simple / LeastInFlight / LeastLatency) ([#20204](#20204)) ([bcbf714](bcbf714)) * **bigtable:** Add ClientConfig.DisableSession to opt out of session backend ([#20297](#20297)) ([7ee5e44](7ee5e44)) * **bigtable:** Add getClientConfigDirectAccessChecker for session pools ([#20209](#20209)) ([3b8d30a](3b8d30a)) * **bigtable:** Add NoOpChannelPrimer for session channel pools ([#20208](#20208)) ([d055a8a](d055a8a)) * **bigtable:** Add per-AFE sessionList for the two-tier session pool ([#20224](#20224)) ([dbf0c3f](dbf0c3f)) * **bigtable:** Add protoRowToRow conversion helper for TableShim ([#20257](#20257)) ([1297143](1297143)) * **bigtable:** Add Session debug surface (observability fields + methods) ([#20211](#20211)) ([d8d3e16](d8d3e16)) * **bigtable:** Add Session lifecycle (Start, Close, ForceClose, readLoop, heartBeatLoop) ([#20215](#20215)) ([b9e53c6](b9e53c6)) * **bigtable:** Add Session struct + state machine ([#20117](#20117)) ([09acbb3](09acbb3)) * **bigtable:** Add session.Config.EnableDebug to gate sessionz debug state ([#20247](#20247)) ([ce74c31](ce74c31)) * **bigtable:** Add SessionClient + SessionTable + lazyPool ([#20228](#20228)) ([ab2c96c](ab2c96c)) * **bigtable:** Add SessionPoolImpl (two-tier pool + scaling + debug) ([#20225](#20225)) ([683eda8](683eda8)) * **bigtable:** Rename session pool display to <resource-id>-<PERM> ([#20248](#20248)) ([35e146e](35e146e)) * **bigtable:** Route Client.Open()-returned *Table through the Diverter ([#20273](#20273)) ([2b81c7d](2b81c7d)) * **bigtable:** State-based classification for abnormal session close ([#20243](#20243)) ([f2905b7](f2905b7)) * **bigtable:** TableShim fallback to classic on session UNIMPLEMENTED ([#20269](#20269)) ([36540af](36540af)) * **bigtable:** TTL-on-idle cache for per-resource session.TableAPI ([#20263](#20263)) ([00b2a49](00b2a49)) * **bigtable:** Wire Diverter on Client and route Open* via TableShim ([#20256](#20256)) ([b32fbd7](b32fbd7)) ### Bug Fixes * **bigtable:** AFE picker latency signal — subtract poolWait and compute TransportLatency = wire − backend at source ([#20281](#20281)) ([bb8c4d5](bb8c4d5)) * **bigtable:** Guard NewStream OnFinish against grpc-go double-fire ([#20295](#20295)) ([b51da29](b51da29)) * **bigtable:** Real per-resource pool teardown on sessionTable.Close + cache close-race gate ([#20264](#20264)) ([599aea9](599aea9)) * **bigtable:** Session.durations / session.uptime — set explicit histogram bucket boundaries ([#20276](#20276)) ([97eee22](97eee22)) * **bigtable:** SessionTableHandle self-heals across cache eviction ([#20296](#20296)) ([0dd98cd](0dd98cd)) * **bigtable:** Translate ctx errors to gRPC status on session vRPC ([#20299](#20299)) ([0f3b2a5](0f3b2a5)) * **bigtable:** Treat PingAndWarm NotFound as a successful prime ([#20219](#20219)) ([a1557ad](a1557ad)) ### Performance Improvements * **bigtable:** Delete periodic Tick loop; sizing is event-driven ([#20285](#20285)) ([2c096bd](2c096bd)) * **bigtable:** Drop pick_lost_race debug tag from CheckoutSession hot path ([#20280](#20280)) ([bd0e400](bd0e400)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
grpc-go v1.82.x can invoke a registered
grpc.OnFinishcallback more than once on some stream-creation failure paths (closedClientConnis the observed one: the retry unwinder fires it once from insidewithRetry, thennewClientStream's deferred stream-teardown fires it again). Compounding that, the oldBigtableChannelPool.NewStreamalso manually decrementedstreamingLoadon the returned-error path assuming OnFinish had NOT fired. Combined:streamingLoadwent 2–3 decrements deep per failed NewStream. Debug pages surfaced this as "Streaming in flight: -9" on an idle classic pool.Fix
Trust
OnFinishas the single source of truth for load accounting + error attribution. Gate the callback body withatomic.Bool.CompareAndSwap(false, true)so it runs exactly once regardless of how many times grpc-go fires it. Belt-and-suspenders: afterentry.conn.NewStreamreturns an error, call the same accounting closure directly — the CAS makes it a no-op if grpc-go already fired, and it covers the zero-fire path if grpc-go ever returns an error without arming OnFinish.Test plan