codeAnqiang-ma opened a new issue, #3654: URL: https://github.com/apache/dubbo-go/issues/3654
### Summary Over the Triple (non-gRPC) unary transport, a server-returned business error carrying `CodeBizError` (17) arrives at the client as `CodeUnknown` (2). The gRPC transport in the same package preserves the code. Because `failover` and the metrics classifier both key off `CodeBizError`, business errors are retried when they should not be, and are counted as unknown failures. ### Environment - Go 1.26.5, macOS (arm64) - dubbo-go `develop` @ `ffbbcc3` - Protocol: Triple (unary over HTTP/1.1, i.e. `WithTriple()`); gRPC framing is unaffected ### Affected Locations | Role | Location | | --- | --- | | Bug | [`protocol/triple/triple_protocol/protocol_triple.go:694`](https://github.com/apache/dubbo-go/blob/develop/protocol/triple/triple_protocol/protocol_triple.go#L690-L697) — `tripleWireError.asError()` clamps any code outside `[minCode, maxCode]` to `CodeUnknown` | | Enum | [`protocol/triple/triple_protocol/code.go:110-113`](https://github.com/apache/dubbo-go/blob/develop/protocol/triple/triple_protocol/code.go#L110-L113) — `CodeBizError = 17`, but `maxCode` is still `CodeUnauthenticated` (16) | | Producer | [`proxy/proxy_factory/default.go:246`](https://github.com/apache/dubbo-go/blob/develop/proxy/proxy_factory/default.go#L246-L249) — wraps application errors in `CodeBizError` | | Consumer | [`cluster/cluster/failover/cluster_invoker.go:113`](https://github.com/apache/dubbo-go/blob/develop/cluster/cluster/failover/cluster_invoker.go#L112-L114) — `isBizError()` skips retry for business errors | | Consumer | [`metrics/rpc/error_classifier.go:126`](https://github.com/apache/dubbo-go/blob/develop/metrics/rpc/error_classifier.go#L120-L130) — maps `CodeBizError` to `ErrorTypeBusinessFailed` | ### Steps to Reproduce Start a handler that returns a business error, then call it over each transport: ```go handler := triple.NewUnaryHandler( "/connect.ping.v1.PingService/Ping", func() any { return new(pingv1.PingRequest) }, func(ctx context.Context, req *triple.Request) (*triple.Response, error) { return nil, triple.NewError(triple.CodeBizError, errors.New("oh no")) }, ) server := httptest.NewServer(handler) client := pingv1connect.NewPingServiceClient(server.Client(), server.URL, triple.WithTriple()) err := client.Ping(context.Background(), triple.NewRequest(&pingv1.PingRequest{Number: 42}), triple.NewResponse(&pingv1.PingResponse{})) // triple.CodeOf(err) => unknown (2) ``` ### Expected Behavior The client observes `CodeBizError`, consistently with the gRPC transport, so that `IsWireError(err) && CodeOf(err) == CodeBizError` holds and `failover` does not retry. ### Actual Behavior | Transport | Client-visible code | `IsWireError && CodeOf == CodeBizError` | | --- | --- | --- | | gRPC over HTTP/2 | `code_17` (17) | ✅ true | | Triple unary over HTTP/1.1 | `unknown` (2) | ❌ false | The server writes the correct thing on the wire — the response body is `{"code":"code_17","message":"oh no"}`. The code is lost on the client while decoding. Consequences: 1. `failover` treats the business error as retryable and retries it (`DefaultRetriesInt = 2`, so up to 3 attempts). Non-idempotent business calls are executed repeatedly. 2. `metrics/rpc.classifyError` falls through to `default` and records `ErrorTypeUnknown` instead of `ErrorTypeBusinessFailed`. ### Root Cause `CodeBizError = 17` is a dubbo-go extension to the code enum inherited from connect-go, which only defines codes 1..16. The clamp in `asError()` was inherited along with the enum and never updated when 17 was added, and it only exists on the Triple unary path — `grpcErrorFromTrailer` builds the error straight from the header value with no clamp, which is why gRPC is unaffected. Worth noting that `code.go` deliberately supports non-canonical codes: `UnmarshalText` has a branch commented > `// Ensure that non-canonical codes round-trip through MarshalText and UnmarshalText.` whose condition (`code < minCode || code > maxCode`) exists precisely so that codes such as 17 survive decoding. `asError()` then discards exactly what `UnmarshalText` just took care to preserve — the two are inconsistent within the same package. ### Possible Solution Exempt `CodeBizError` from the wire-code clamp in `asError()`: ```go if (e.Code < minCode || e.Code > maxCode) && e.Code != CodeBizError { e.Code = CodeUnknown } ``` Raising `maxCode` to `CodeBizError` instead looks more principled but breaks the round-trip: the `UnmarshalText` fallback only keeps codes *outside* `[minCode, maxCode]`, so `code_17` would start failing with `invalid code`. Making that work needs a canonical string for `CodeBizError` in both `String()` and `UnmarshalText`, which changes the wire representation. I have the one-line fix and a regression test (real `httptest` server, asserting both transports) ready and will open a PR against `develop` referencing this issue. _This report was prepared with AI assistance; the author reproduced and reviewed every conclusion locally._ -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
