Repository navigation
fix(triple): preserve CodeBizError over the Triple unary transport - #3655
Merged
Alanxtl merged 1 commit intoAug 16, 2026
Merged
Conversation
Over the Triple (non-gRPC) unary transport, tripleWireError.asError() clamped any code outside [minCode, maxCode] to CodeUnknown, so a server-returned CodeBizError (17) reached the client as CodeUnknown (2). The clamp comes from connect-go, which only defines codes 1..16; CodeBizError is a dubbo-go extension and maxCode was never updated. code.go already goes out of its way to keep such codes intact, so that UnmarshalText round-trips them, and asError then discarded them again. As a result failover.isBizError stopped recognizing business errors and retried them, and metrics/rpc counted them as unknown failures. The gRPC transport was unaffected because grpcErrorFromTrailer does not clamp. Exempt CodeBizError from the clamp and add a regression test asserting the client-visible code over both transports. Fixes: apache#3654 Assisted-by: Cursor (Claude Opus 5) Signed-off-by: codeAnqiang-ma <273298913+codeAnqiang-ma@users.noreply.github.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3655 +/- ##
===========================================
+ Coverage 46.76% 54.57% +7.81%
===========================================
Files 295 474 +179
Lines 17172 36701 +19529
===========================================
+ Hits 8031 20031 +12000
- Misses 8287 15050 +6763
- Partials 854 1620 +766 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a Triple (non-gRPC) unary transport decoding issue where a server-returned CodeBizError (17) was being clamped to CodeUnknown, aligning behavior with the gRPC transport and preventing downstream retry/metrics misclassification.
Changes:
- Preserve
CodeBizErrorintripleWireError.asError()by exempting it from the out-of-range clamp. - Add a regression test that spins up real
httptestservers and asserts the client-visible code is preserved over both Triple and gRPC unary transports.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| protocol/triple/triple_protocol/protocol_triple.go | Exempts CodeBizError from the wire-code clamp during error reconstruction. |
| protocol/triple/triple_protocol/triple_ext_test.go | Adds an end-to-end regression test verifying CodeBizError survives across both transports. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+723
to
+732
| assertBizError := func(t *testing.T, server *httptest.Server, opts ...triple.ClientOption) { //nolint:thelper | ||
| client := pingv1connect.NewPingServiceClient(server.Client(), server.URL, opts...) | ||
| request := triple.NewRequest(&pingv1.PingRequest{Number: 42}) | ||
| response := triple.NewResponse(&pingv1.PingResponse{}) | ||
| err := client.Ping(context.Background(), request, response) | ||
| assert.NotNil(t, err) | ||
| var tripleErr *triple.Error | ||
| assert.True(t, errors.As(err, &tripleErr)) | ||
| assert.Equal(t, tripleErr.Code(), triple.CodeBizError) | ||
| } |
Member
|
cc @CAICAIIs , help review this please |
AlexStocks
approved these changes
Aug 14, 2026
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
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.
Description
Fixes #3654
Over the Triple (non-gRPC) unary transport, a server-returned
CodeBizError(17) reaches the client asCodeUnknown(2); the gRPC transport in the same package preserves it. Downstream,failover.isBizErrorstops recognizing business errors and retries them (default up to 3 attempts, so non-idempotent calls run repeatedly), andmetrics/rpccounts them as unknown.Root cause.
tripleWireError.asError()(protocol_triple.go:694) clamps any code outside[minCode, maxCode]=[1,16]toCodeUnknown. That clamp comes from connect-go, which only defines codes 1..16;CodeBizError = 17is a dubbo-go extension andmaxCodewas never updated.code.godeliberately supports such codes — theUnmarshalTextbranch commented "Ensure that non-canonical codes round-trip through MarshalText and UnmarshalText" keeps out-of-range codes for exactly this reason.asError()then discards whatUnmarshalTextjust preserved.Fix. Exempt
CodeBizErrorfrom the clamp (one line), plusTestBizErrorCodePreservedAcrossProtocols: a realhttptestserver whose handler returnsNewError(CodeBizError, ...), asserting the client-visible code over both transports. Before the fix thetriplesubtest fails (got: unknown, want: code_17) whilegrpcpasses; after, both pass.Why not raise
maxCodetoCodeBizErrorinsteadThat breaks the round-trip. The
UnmarshalTextfallback only keeps codes outside[minCode, maxCode]:With
maxCode = CodeBizError, the stringcode_17no longer satisfies that condition and falls through toinvalid code %q. Making it work needs a canonical string forCodeBizErrorin bothString()andUnmarshalText, which changes the wire representation of the code. The clamp exemption keeps the existing wire format and touches one line.Test output (before / after)
Before the fix — only
protocol_triple.goreverted, test unchanged:After:
Existing tests, including the packages that consume the code:
go vet ./protocol/triple/triple_protocol/...andgofmt -lare both clean.Not run locally: the full
make testand the integration suite (they need ZooKeeper / Nacos / etcd / Docker, which I don't have set up), and golangci-lint. Leaving those to CI.Checklist
developAssisted-by: Cursor (Claude Opus 5). Reproduced, reviewed and tested locally by the author.