locate: fail fast on TLS authentication errors instead of backoffer retry#1972
locate: fail fast on TLS authentication errors instead of backoffer retry#1972xhebox wants to merge 1 commit into
Conversation
…etry When a TiDB cluster table (e.g. CLUSTER_TIDB_TRX) fans out to a peer TiDB node whose advertise-address is empty, the status address becomes :10080. gRPC normalizes this to localhost:10080, causing TLS ServerName = "localhost" which fails certificate verification. Previously, this TLS error was treated as a transient RPC failure and retried by the backoffer until the 40000ms budget was exhausted, turning an instant configuration bug into a ~40s query hang. This commit detects TLS authentication errors in onSendFail and returns them immediately without retry, since certificate mismatches are permanent configuration issues that will not recover by waiting. Signed-off-by: xhe <xw897002528@gmail.com>
ti-chi-bot
Bot
added
the
dco-signoff: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughRegion request sending now detects TLS/authentication certificate handshake failures and treats them as non-retriable permanent errors, short-circuiting the retry/backoff logic. A new helper function identifies such failures, ChangesTLS Authentication Fast-Fail
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ti-chi-bot
Bot
added
the
size/M
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/locate/region_request_test.go (1)
1285-1291: ⚡ Quick winAsync retry assertion is currently non-verifying.
retryTimesisn’t a reliable signal in AsyncAPI mode here:SendReqCtxreturns0unconditionally on the failpoint async path (Line 1493 ininternal/locate/region_request.go). Please assert actual send attempts (client fn call count) to prove fail-fast/no-retry behavior.Proposed test hardening
test := func() { oc := s.regionRequestSender.client defer func() { s.regionRequestSender.client = oc }() + attempts := 0 // Simulate the gRPC error produced when TLS ServerName "localhost" does not match cert SANs. tlsErr := status.Error(codes.Unavailable, `connection error: desc = "transport: authentication handshake failed: `+ `tls: failed to verify certificate: x509: certificate is valid for example.com, not localhost"`) s.regionRequestSender.client = &fnClient{fn: func(ctx context.Context, addr string, req *tikvrpc.Request, timeout time.Duration) (*tikvrpc.Response, error) { + attempts++ return nil, tlsErr }} bo := retry.NewBackofferWithVars(context.Background(), 5, nil) resp, retryTimes, err := s.regionRequestSender.SendReq(bo, req, region.Region, time.Second) s.Nil(resp) s.NotNil(err) // TLS auth error should fail fast without any retry. s.Zero(retryTimes) + s.Equal(1, attempts) s.Contains(err.Error(), "x509:") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/locate/region_request_test.go` around lines 1285 - 1291, The test currently relies on retryTimes (returned by SendReq) which is non-verifying in the async failpoint path; instead modify the test to assert the actual number of send attempts by inspecting the mock client function call count used by regionRequestSender (e.g. the mock send/ClientFn you registered) after calling SendReq/SendReqCtx, verify it is exactly 1 (or 0 depending on expected fail-fast) to prove no retries, keep the existing assertions for resp nil and error containing "x509:", and remove or de-emphasize reliance on retryTimes returned by SendReq/SendReqCtx.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/locate/region_request_test.go`:
- Around line 1285-1291: The test currently relies on retryTimes (returned by
SendReq) which is non-verifying in the async failpoint path; instead modify the
test to assert the actual number of send attempts by inspecting the mock client
function call count used by regionRequestSender (e.g. the mock send/ClientFn you
registered) after calling SendReq/SendReqCtx, verify it is exactly 1 (or 0
depending on expected fail-fast) to prove no retries, keep the existing
assertions for resp nil and error containing "x509:", and remove or de-emphasize
reliance on retryTimes returned by SendReq/SendReqCtx.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 05351ff1-e215-4037-832a-5200b4e138d4
📒 Files selected for processing (2)
internal/locate/region_request.gointernal/locate/region_request_test.go
1 participant
What is changed and how it works?
When a TiDB cluster table (e.g. CLUSTER_TIDB_TRX) fans out to a peer TiDB node whose advertise-address is empty, the status address becomes :10080. gRPC normalizes this to localhost:10080, causing TLS ServerName = "localhost" which fails certificate verification.
Previously, this TLS error was treated as a transient RPC failure and retried by the backoffer until the 40000ms budget was exhausted, turning an instant configuration bug into a ~40s query hang.
This PR detects TLS authentication errors in onSendFail and returns them immediately without retry, since certificate mismatches are permanent configuration issues that will not recover by waiting.
ref pingcap/tidb#68519
Changes
Check List
Tests
Side effects
Related changes
Release note
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests