fix(bigtable): translate ctx errors to gRPC status on session vRPC - #20299
Merged
sushanb merged 6 commits intoAug 3, 2026
Merged
Conversation
awaitInvokeResult's ctx.Done branch wraps ctx.Err() in tagErr(...) → *vrpcErr. Neither context.DeadlineExceeded nor *vrpcErr implemented GRPCStatus(), so status.Code(err) on the returned error surfaced as codes.Unknown even when the actual cause was a client-side deadline or cancellation. Observable symptom: tight-deadline probes against real Bigtable (10ms / 2ms ctx) produced errors that read "session ReadRow vRPC: context deadline exceeded" but status.Code reported UNKNOWN, breaking upstream code (session_pool.go:604 uses errors.Is but any consumer keying off the gRPC code was misclassifying these attempts). The classic gRPC unary path never hits this because grpc-go's client-side interceptor translates ctx.Err() to a *status.Status before returning. The session vRPC path bypasses that interceptor (bidi-stream with hand-managed request routing) so translation has to happen in-package. Fix: add GRPCStatus() to *vrpcErr. Preserves an existing status if present; otherwise uses status.FromContextError to translate stdlib ctx errors; otherwise falls back to Unknown with the underlying message. Unwrap() is untouched so errors.Is(err, context.DeadlineExceeded) at session_pool.go:604 and session_pool_debug.go:337 keeps working. Tests: extended the two existing ctx-deadline tests in session_vrpc_test.go with status.Code assertions. Verified both FAIL on main (Unknown) and PASS with this change.
Contributor
There was a problem hiding this comment.
Code Review
This pull request implements the GRPCStatus method on vrpcErr to ensure that wrapped context errors and other gRPC errors are correctly translated to their corresponding gRPC status codes. Tests are also updated to verify this behavior. The reviewer pointed out that status.FromContextError does not unwrap errors, meaning wrapped context errors would incorrectly resolve to codes.Unknown. They suggested explicitly checking for context.Canceled and context.DeadlineExceeded using errors.Is to ensure correct translation.
Comment on lines
+101
to
+103
| if s := status.FromContextError(e.outcome.Err); s.Code() != codes.Unknown { | ||
| return s | ||
| } |
Contributor
There was a problem hiding this comment.
While status.FromError natively unwraps errors using errors.As to find a gRPC status, status.FromContextError does not perform any unwrapping (it only does a direct equality check against context.Canceled and context.DeadlineExceeded). If e.outcome.Err is a wrapped context error (e.g., wrapped via fmt.Errorf), status.FromContextError will fail to recognize it and return codes.Unknown.
To ensure wrapped context errors are correctly translated, use errors.Is to check for context.Canceled and context.DeadlineExceeded explicitly. Note that you will need to import the "context" package in this file.
if errors.Is(e.outcome.Err, context.Canceled) {
return status.New(codes.Canceled, e.outcome.Err.Error())
}
if errors.Is(e.outcome.Err, context.DeadlineExceeded) {
return status.New(codes.DeadlineExceeded, e.outcome.Err.Error())
}
Contributor
Author
There was a problem hiding this comment.
Verified against the vendored grpc-go source (v1.68.0, status/status.go:151-163) — FromContextError uses errors.Is, not identity comparison:
func FromContextError(err error) *Status {
if err == nil { return nil }
if errors.Is(err, context.DeadlineExceeded) {
return New(codes.DeadlineExceeded, err.Error())
}
if errors.Is(err, context.Canceled) {
return New(codes.Canceled, err.Error())
}
return New(codes.Unknown, err.Error())
}The doc comment even leads with "converts a context error or wrapped context error". Wrapped ctx errors resolve correctly today.
The TestVRPCErr_GRPCStatus sub-test at attempt_outcome_test.go explicitly pins this via the fmt.Errorf-wrapped ctx err still translates via errors.Is walk case — it constructs fmt.Errorf("send vRPC request: %w", context.DeadlineExceeded) and asserts GRPCStatus().Code() == codes.DeadlineExceeded. Passes on 5dced07.
That sub-test is deliberately there as regression insurance against exactly the scenario you're flagging — if a future grpc-go version reverts to identity comparison, the test fails and we'd add explicit errors.Is here. But under current grpc-go, adding explicit checks would be duplicating what FromContextError already does.
… comment - Add TestVRPCErr_GRPCStatus covering all three preference branches plus the fmt.Errorf-wrapped ctx err shape produced by call sites like session_vrpc.go:163. Guards against a future grpc-go bump that swaps FromContextError's errors.Is walk for identity comparison. - Delete the now-redundant switch at session_pool.go:599 that hand- translated ctx errors to string codes. status.Code(invokeErr) is now correct for tagged ctx errors, so ev.ErrCode = status.Code(...) .String() is enough. Removes the stale comment claiming ctx errors don't implement GRPCStatus. - Drop the unreachable nil-Err branch in GRPCStatus. tagErr returns nil for nil err, so a live *vrpcErr always has non-nil outcome.Err (Error() and Unwrap() already rely on this invariant). - Trim the 14-line method comment on GRPCStatus down to 6 lines. Drop the stale "underlying err" claim from the TagErr docstring.
Cut redundant/explanatory prose per CLAUDE.md conventions (only non-obvious WHY, no cross-file line references, don't restate what the code says): - GRPCStatus doc: 7 lines → 4 (drop branch-order restatement). - TagErr doc: 4 lines → 2 (collapse to Unwrap + GRPCStatus contract). - TestVRPCErr_GRPCStatus doc: 9 lines → 4 (drop redundant WHAT, drop the session_vrpc.go:163 reference — line numbers rot). - session_vrpc_test.go: delete the 5-line inline explanation on the first status.Code assertion; the second assertion doesn't have one, and the assertion's failure message already carries the intent.
mutianf
reviewed
Aug 3, 2026
| if s, ok := status.FromError(e.outcome.Err); ok { | ||
| return s | ||
| } | ||
| if s := status.FromContextError(e.outcome.Err); s.Code() != codes.Unknown { |
Contributor
There was a problem hiding this comment.
why check Unknown code specifically and wrap it again later? wouldnt this hide the original error's stacktrace?
status.FromContextError's contract already returns Unknown-with-message for non-ctx errors, which is identical to the explicit fallback we had. Two branches suffice: existing-status wins, else defer to FromContextError. Drops the now-unused codes import.
Igor-reviewer noted that session_pool_debug.go:336 had the same hand-rolled ctx-err → code switch that this PR just deleted from SessionPoolImpl.Invoke. Same bug shape, same fix. - New statusOf(err) helper in attempt_outcome.go: existing gRPC status wins, else translate via status.FromContextError. Same two-branch logic vrpcErr.GRPCStatus already had. - vrpcErr.GRPCStatus now a one-liner delegating to statusOf. - session_pool.go slow-vRPC labeler uses statusOf too (works either way since invokeErr is *vrpcErr, but keeps both call sites expressing the same intent). - session_pool_debug.go slow-checkout labeler: 8-line switch → one line. Drops now-unused context / errors / status imports.
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>
3 tasks
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
awaitInvokeResult'sctx.Donebranch (bigtable/internal/transport/session_vrpc.go:242) wrapsctx.Err()intagErr(...)→*vrpcErr. Neithercontext.DeadlineExceedednor*vrpcErrimplementedGRPCStatus(), sostatus.Code(err)on the returned error surfaced ascodes.Unknowneven when the actual cause was a client-side deadline or cancellation.Observable symptom: a tight-deadline probe against real Bigtable (10ms per-RPC ctx,
CBT_FORCE_SESSION=true) produced errors that readsession ReadRow vRPC: context deadline exceeded— butstatus.Code(err)reportedUNKNOWN, misleading any consumer keying off the gRPC code (retry classifiers, dashboards, log labels).The classic gRPC unary path never hits this: grpc-go's client-side interceptor auto-translates
ctx.Err()to a*status.Statusbefore returning. The session vRPC path is a bidi stream with hand-managed request routing and bypasses that interceptor, so the translation has to happen in-package.The fix
Add
GRPCStatus()to*vrpcErrinattempt_outcome.go:status.FromContextError(...)— translatescontext.DeadlineExceeded→DeadlineExceeded,context.Canceled→Canceled.Unknownwith the underlying message.Placed on
*vrpcErr(not at the specific call site) because every ctx-cancel error path in the transport package flows throughtagErr→*vrpcErr, and this also covers thesendErrpath atsession_vrpc.go:163if a synchronous send ever surfaces a ctx-based error.Unwrap()is untouched, soerrors.Is(err, context.DeadlineExceeded)atsession_pool.go:604andsession_pool_debug.go:337keeps working.Test plan
session_vrpc_test.go(TestHandleVRPCResponse_LateResponseAfterCtxDone_FlagsCancelledDrained,TestInvoke_SecondInvokeAfterCtxDoneRejectedUncommitted) withstatus.Code == DeadlineExceededassertions alongside the existingerrors.Ischecks.main(status.Code(err) = Unknown, want DeadlineExceeded) and PASS with this change.go test ./internal/transport/ -count=1 -short -timeout=180s— all green (21s).go vet ./internal/transport/— clean.