From bc2020804b633b1f8c0f644dcf0cd1d102c35645 Mon Sep 17 00:00:00 2001 From: Marketen Date: Thu, 15 Oct 2026 09:10:32 +0200 Subject: [PATCH] fix: keep the provider's status and meaning in upstream errors Every provider 4xx came back as 502 provider_unavailable, so a request the provider rejected looked like an outage, and our own provider credential or billing failures looked the same as both. - Provider 4xx keeps its status, code provider_rejected_request. - Provider 401/402/403 become 502 provider_configuration_error: they concern Nexus's provider account, not the caller's key. - 429 becomes provider_rate_limited and keeps a validated Retry-After. - 504 becomes provider_timeout; other 5xx keep their status as provider_error. - Errors carry origin (gateway or upstream) and upstream_status. Other diagnostic metadata stays internal. - A configured fallback provider is tried only after 429 or 5xx. Co-Authored-By: Claude Opus 5.5 --- README.md | 10 +++++ .../adapters/http/dto/error_response.go | 8 ++-- .../internal/adapters/http/handlers/common.go | 31 ++++++++++----- .../http/handlers/common_error_test.go | 38 ++++++++++++++++++ .../adapters/providers/openai/adapter.go | 16 ++++++-- .../adapters/providers/openai/client.go | 21 ++++++++-- .../providers/openai/error_mapping_test.go | 37 ++++++++++++++++++ .../providers/openai/retry_after_test.go | 19 +++++++++ .../adapters/providers/tinfoil/adapter.go | 4 +- .../application/services/generate_service.go | 7 +++- .../services/generate_service_test.go | 14 +++++++ pkg/domain/error.go | 39 +++++++++++-------- 12 files changed, 203 insertions(+), 41 deletions(-) create mode 100644 apps/gateway/internal/adapters/http/handlers/common_error_test.go create mode 100644 apps/gateway/internal/adapters/providers/openai/error_mapping_test.go create mode 100644 apps/gateway/internal/adapters/providers/openai/retry_after_test.go diff --git a/README.md b/README.md index b1c509d..1900755 100644 --- a/README.md +++ b/README.md @@ -75,6 +75,16 @@ tool calls, parallel tool calls, structured outputs and reasoning. The current capabilities for each model are shown in the [model catalog](https://nexus.dappnode.com/models). +Provider errors keep their meaning. A request the provider rejects keeps its +HTTP status (for example 400 or 422) with `provider_rejected_request`. A +provider credential, billing or permission failure becomes 502 +`provider_configuration_error`, because it concerns Nexus's provider account, +not the caller's key. Provider rate limits become `provider_rate_limited` and +keep a valid `Retry-After` header. Errors include `origin` (`gateway` or +`upstream`) and, when known, the numeric `upstream_status`; other diagnostic +details are not exposed. A configured fallback provider is tried only after a +rate limit or a server-side failure, never after a rejected request. + ## Privacy and usage controls Nexus provides per-key PII masking settings and access to privacy-focused diff --git a/apps/gateway/internal/adapters/http/dto/error_response.go b/apps/gateway/internal/adapters/http/dto/error_response.go index 689ad79..abc23de 100644 --- a/apps/gateway/internal/adapters/http/dto/error_response.go +++ b/apps/gateway/internal/adapters/http/dto/error_response.go @@ -7,7 +7,9 @@ type ErrorResponse struct { // ErrorDetail contains the error classification fields. type ErrorDetail struct { - Type string `json:"type"` - Code string `json:"code"` - Message string `json:"message"` + Type string `json:"type"` + Code string `json:"code"` + Message string `json:"message"` + Origin string `json:"origin"` + UpstreamStatus int `json:"upstream_status,omitempty"` } diff --git a/apps/gateway/internal/adapters/http/handlers/common.go b/apps/gateway/internal/adapters/http/handlers/common.go index 9563b5e..f2a67e2 100644 --- a/apps/gateway/internal/adapters/http/handlers/common.go +++ b/apps/gateway/internal/adapters/http/handlers/common.go @@ -26,12 +26,11 @@ func WriteError(w http.ResponseWriter, err error) { if !ok { gwErr = domain.ErrInternal("an internal error occurred") } + if gwErr.RetryAfter != "" { + w.Header().Set("Retry-After", gwErr.RetryAfter) + } WriteJSON(w, gwErr.HTTPStatus, dto.ErrorResponse{ - Error: dto.ErrorDetail{ - Type: gwErr.Type, - Code: gwErr.Code, - Message: gwErr.Message, - }, + Error: errorDetail(gwErr), }) } @@ -69,15 +68,27 @@ func WriteErrorWithLog(w http.ResponseWriter, r *http.Request, logger ports.Logg ) } + if gwErr.RetryAfter != "" { + w.Header().Set("Retry-After", gwErr.RetryAfter) + } WriteJSON(w, gwErr.HTTPStatus, dto.ErrorResponse{ - Error: dto.ErrorDetail{ - Type: gwErr.Type, - Code: gwErr.Code, - Message: gwErr.Message, - }, + Error: errorDetail(gwErr), }) } +// Diagnostic extensions expose only classification and numeric status. +// Arbitrary internal metadata is never serialized to clients. +func errorDetail(err *domain.GatewayError) dto.ErrorDetail { + detail := dto.ErrorDetail{Type: err.Type, Code: err.Code, Message: err.Message, Origin: "gateway"} + if err.Type == domain.ErrTypeProvider { + detail.Origin = "upstream" + } + if status, ok := err.Metadata["upstream_status"].(int); ok { + detail.UpstreamStatus = status + } + return detail +} + // ExtractBearerToken extracts the bearer token from the Authorization header. func ExtractBearerToken(r *http.Request) (string, error) { auth := r.Header.Get("Authorization") diff --git a/apps/gateway/internal/adapters/http/handlers/common_error_test.go b/apps/gateway/internal/adapters/http/handlers/common_error_test.go new file mode 100644 index 0000000..c9fc86d --- /dev/null +++ b/apps/gateway/internal/adapters/http/handlers/common_error_test.go @@ -0,0 +1,38 @@ +package handlers + +import ( + "encoding/json" + "net/http/httptest" + "strings" + "testing" + + "github.com/dappnode/dappnode-nexus-gateway/apps/gateway/internal/adapters/http/dto" + "github.com/dappnode/dappnode-nexus-gateway/pkg/domain" +) + +func TestErrorsExposeOriginAndStatusWithoutInternalMetadata(t *testing.T) { + upstream := domain.ErrProviderError(429, "rate limited").WithMeta("upstream_status", 429, "upstream_error", "PRIVATE-DIAGNOSTIC-CANARY") + upstream.RetryAfter = "17" + for _, tc := range []struct { + err *domain.GatewayError + origin string + status int + retry string + }{ + {upstream, "upstream", 429, "17"}, + {domain.ErrInvalidField("invalid input"), "gateway", 0, ""}, + } { + recorder := httptest.NewRecorder() + WriteError(recorder, tc.err) + var got dto.ErrorResponse + if err := json.Unmarshal(recorder.Body.Bytes(), &got); err != nil { + t.Fatal(err) + } + if recorder.Code != tc.err.HTTPStatus || got.Error.Origin != tc.origin || got.Error.UpstreamStatus != tc.status || recorder.Header().Get("Retry-After") != tc.retry { + t.Fatalf("error classification: %#v", got) + } + if strings.Contains(recorder.Body.String(), "PRIVATE-DIAGNOSTIC-CANARY") { + t.Fatal("exposed internal diagnostic metadata") + } + } +} diff --git a/apps/gateway/internal/adapters/providers/openai/adapter.go b/apps/gateway/internal/adapters/providers/openai/adapter.go index 8a87290..1f6710a 100644 --- a/apps/gateway/internal/adapters/providers/openai/adapter.go +++ b/apps/gateway/internal/adapters/providers/openai/adapter.go @@ -60,23 +60,29 @@ func mapProviderError(err error, providerName string) error { if errors.As(err, &httpErr) { var gwErr *domain.GatewayError switch { - case httpErr.StatusCode == 401 || httpErr.StatusCode == 403: + case httpErr.StatusCode == 401 || httpErr.StatusCode == 402 || httpErr.StatusCode == 403: // Auth / permission / billing failure at the upstream provider. // Novita uses 403 for invalid API key, insufficient balance, and access denied. gwErr = domain.ErrProviderError(502, fmt.Sprintf("provider auth/permission error: %s", httpErr.Body)) + gwErr.Code = domain.ErrCodeProviderConfiguration case httpErr.StatusCode == 429: // Rate limit or token limit exceeded — surface to client so it can back off. gwErr = domain.ErrProviderError(429, "provider rate limited: "+httpErr.Body) + gwErr.Code = domain.ErrCodeProviderRateLimited case httpErr.StatusCode == 503: // Service unavailable — surface as 503 so clients know to retry. gwErr = domain.ErrProviderUnavailable(providerName) + case httpErr.StatusCode == 504: + gwErr = domain.ErrProviderTimeout(providerName) case httpErr.StatusCode >= 500: - gwErr = domain.ErrProviderError(502, fmt.Sprintf("provider server error: %s", httpErr.Body)) + gwErr = domain.ErrProviderError(httpErr.StatusCode, fmt.Sprintf("provider server error: %s", httpErr.Body)) default: // Client errors from upstream (400, 404, 422, etc.) — the gateway // forwarded a request the provider doesn't accept. - gwErr = domain.ErrProviderError(502, fmt.Sprintf("provider rejected request: %s", httpErr.Body)) + gwErr = domain.ErrProviderError(httpErr.StatusCode, fmt.Sprintf("provider rejected request: %s", httpErr.Body)) + gwErr.Code = domain.ErrCodeProviderRejectedRequest } + gwErr.RetryAfter = httpErr.RetryAfter return gwErr.WithMeta( "upstream_status", httpErr.StatusCode, "upstream_error", httpErr.Body, @@ -209,7 +215,9 @@ func mapProviderErrorWithCompatibilityContext(err error, model domain.PublicMode if model.ProviderConfig.ProviderName == "novita" && isInvalidRequestHTTPError(err) && hasNonDroppableToolChoice(body) { var httpErr *ProviderHTTPError if errors.As(err, &httpErr) { - return domain.ErrProviderError(502, "provider rejected request; Novita may not support the requested tool_choice semantics for this model: "+httpErr.Body).WithMeta( + mapped := mapProviderError(err, model.ProviderConfig.ProviderName).(*domain.GatewayError) + mapped.Message = "provider rejected request containing required or named tool_choice: " + httpErr.Body + return mapped.WithMeta( "upstream_status", httpErr.StatusCode, "upstream_error", httpErr.Body, "upstream_type", httpErr.Type, diff --git a/apps/gateway/internal/adapters/providers/openai/client.go b/apps/gateway/internal/adapters/providers/openai/client.go index 32145ee..cf29144 100644 --- a/apps/gateway/internal/adapters/providers/openai/client.go +++ b/apps/gateway/internal/adapters/providers/openai/client.go @@ -10,6 +10,7 @@ import ( "net" "net/http" "regexp" + "strconv" "strings" "time" @@ -77,7 +78,7 @@ func (c *Client) DoEndpoint(ctx context.Context, baseURL, endpoint, apiKey strin } if resp.StatusCode >= 400 { - return nil, parseProviderError(resp.StatusCode, respBody) + return nil, ParseProviderResponseError(resp, respBody) } return respBody, nil @@ -104,7 +105,7 @@ func (c *Client) DoStream(ctx context.Context, baseURL, apiKey string, body map[ if resp.StatusCode >= 400 { body, _ := io.ReadAll(io.LimitReader(resp.Body, 10*1024*1024)) resp.Body.Close() - return nil, parseProviderError(resp.StatusCode, body) + return nil, ParseProviderResponseError(resp, body) } return resp, nil @@ -113,6 +114,7 @@ func (c *Client) DoStream(ctx context.Context, baseURL, apiKey string, body map[ // ProviderHTTPError carries the upstream HTTP status code so the gateway can // propagate a meaningful status instead of always returning 502. type ProviderHTTPError struct { + RetryAfter string StatusCode int Body string RawBody string @@ -182,8 +184,19 @@ func parseProviderError(statusCode int, body []byte) error { } } -func ParseProviderError(statusCode int, body []byte) error { - return parseProviderError(statusCode, body) +// ParseProviderResponseError also keeps a valid Retry-After value (seconds or +// an HTTP date). Other provider headers are never forwarded. +func ParseProviderResponseError(resp *http.Response, body []byte) error { + err := parseProviderError(resp.StatusCode, body).(*ProviderHTTPError) + value := strings.TrimSpace(resp.Header.Get("Retry-After")) + if len(value) <= 128 && !strings.ContainsAny(value, "\r\n") { + if _, parseErr := strconv.ParseUint(value, 10, 32); parseErr == nil { + err.RetryAfter = value + } else if _, parseErr := http.ParseTime(value); parseErr == nil { + err.RetryAfter = value + } + } + return err } func stringifyProviderCode(v any) string { diff --git a/apps/gateway/internal/adapters/providers/openai/error_mapping_test.go b/apps/gateway/internal/adapters/providers/openai/error_mapping_test.go new file mode 100644 index 0000000..2d6f09d --- /dev/null +++ b/apps/gateway/internal/adapters/providers/openai/error_mapping_test.go @@ -0,0 +1,37 @@ +package openai + +import ( + "context" + "errors" + "testing" + + "github.com/dappnode/dappnode-nexus-gateway/pkg/domain" +) + +func TestUpstreamErrorClassification(t *testing.T) { + for _, tc := range []struct { + upstream, downstream int + code string + }{ + {400, 400, domain.ErrCodeProviderRejectedRequest}, + {404, 404, domain.ErrCodeProviderRejectedRequest}, + {422, 422, domain.ErrCodeProviderRejectedRequest}, + {401, 502, domain.ErrCodeProviderConfiguration}, + {402, 502, domain.ErrCodeProviderConfiguration}, + {403, 502, domain.ErrCodeProviderConfiguration}, + {429, 429, domain.ErrCodeProviderRateLimited}, + {500, 500, domain.ErrCodeProviderError}, + {502, 502, domain.ErrCodeProviderError}, + {503, 503, domain.ErrCodeProviderUnavailable}, + {504, 504, domain.ErrCodeProviderTimeout}, + } { + err := mapProviderError(&ProviderHTTPError{StatusCode: tc.upstream, Body: "rejected"}, "provider") + var got *domain.GatewayError + if !errors.As(err, &got) || got.HTTPStatus != tc.downstream || got.Code != tc.code || got.Metadata["upstream_status"] != tc.upstream { + t.Fatalf("upstream %d: %#v", tc.upstream, got) + } + } + if got := mapProviderError(context.Canceled, "provider").(*domain.GatewayError); got.Code != domain.ErrCodeClientCanceled { + t.Fatal(got) + } +} diff --git a/apps/gateway/internal/adapters/providers/openai/retry_after_test.go b/apps/gateway/internal/adapters/providers/openai/retry_after_test.go new file mode 100644 index 0000000..05b5967 --- /dev/null +++ b/apps/gateway/internal/adapters/providers/openai/retry_after_test.go @@ -0,0 +1,19 @@ +package openai + +import ( + "net/http" + "testing" +) + +func TestRetryGuidanceIsPreservedWithoutForwardingArbitraryHeaders(t *testing.T) { + for _, tc := range []struct { + value string + valid bool + }{{"17", true}, {"Wed, 21 Oct 2015 07:28:00 GMT", true}, {"-1", false}, {"secret\r\nX-Secret: value", false}, {"provider-message", false}} { + resp := &http.Response{StatusCode: 429, Header: http.Header{"Retry-After": []string{tc.value}}} + err := ParseProviderResponseError(resp, []byte(`{"error":{"message":"rate limit"}}`)).(*ProviderHTTPError) + if (err.RetryAfter != "") != tc.valid { + t.Fatalf("retry guidance %q: %q", tc.value, err.RetryAfter) + } + } +} diff --git a/apps/gateway/internal/adapters/providers/tinfoil/adapter.go b/apps/gateway/internal/adapters/providers/tinfoil/adapter.go index 3b57a71..3f06017 100644 --- a/apps/gateway/internal/adapters/providers/tinfoil/adapter.go +++ b/apps/gateway/internal/adapters/providers/tinfoil/adapter.go @@ -101,7 +101,7 @@ func (a *Adapter) do(ctx context.Context, verified VerifiedClient, apiKey string return nil, fmt.Errorf("failed to read Tinfoil response: %w", err) } if resp.StatusCode >= 400 { - return nil, openai.ParseProviderError(resp.StatusCode, respBody) + return nil, openai.ParseProviderResponseError(resp, respBody) } return respBody, nil } @@ -118,7 +118,7 @@ func (a *Adapter) doStream(ctx context.Context, verified VerifiedClient, apiKey if resp.StatusCode >= 400 { respBody, _ := io.ReadAll(io.LimitReader(resp.Body, 10*1024*1024)) resp.Body.Close() - return nil, openai.ParseProviderError(resp.StatusCode, respBody) + return nil, openai.ParseProviderResponseError(resp, respBody) } return resp, nil } diff --git a/apps/gateway/internal/application/services/generate_service.go b/apps/gateway/internal/application/services/generate_service.go index 6be0ca9..0f68140 100644 --- a/apps/gateway/internal/application/services/generate_service.go +++ b/apps/gateway/internal/application/services/generate_service.go @@ -74,8 +74,13 @@ func shouldTryFallback(ctx context.Context, err error, fallback *domain.Provider if err == nil || fallback == nil || ctx.Err() != nil { return false } + // A provider that rejected the request would reject it from the fallback + // too; only rate limits and server-side failures change provider. var gatewayErr *domain.GatewayError - return !errors.As(err, &gatewayErr) || gatewayErr.Code != domain.ErrCodeClientCanceled + if !errors.As(err, &gatewayErr) { + return true + } + return gatewayErr.HTTPStatus == 429 || gatewayErr.HTTPStatus >= 500 } func withProviderTarget(model domain.PublicModel, target domain.ProviderTarget) domain.PublicModel { diff --git a/apps/gateway/internal/application/services/generate_service_test.go b/apps/gateway/internal/application/services/generate_service_test.go index 70e63ff..f98f360 100644 --- a/apps/gateway/internal/application/services/generate_service_test.go +++ b/apps/gateway/internal/application/services/generate_service_test.go @@ -568,3 +568,17 @@ func TestProxy_ReturnsRouterErrorForExplicitRouterOutage(t *testing.T) { t.Fatalf("router calls = %d, want 1", router.calls) } } + +func TestRequestRejectionDoesNotChangeProvider(t *testing.T) { + fallback := &domain.ProviderTarget{} + for _, status := range []int{400, 404, 413, 422, 499} { + if shouldTryFallback(context.Background(), domain.ErrProviderError(status, "rejected"), fallback) { + t.Fatalf("fell back after %d", status) + } + } + for _, err := range []error{domain.ErrProviderUnavailable("fixture"), domain.ErrProviderError(429, "rate limited"), domain.ErrProviderError(500, "failed")} { + if !shouldTryFallback(context.Background(), err, fallback) { + t.Fatalf("%v did not permit configured fallback", err) + } + } +} diff --git a/pkg/domain/error.go b/pkg/domain/error.go index 0382d9a..34a1a2d 100644 --- a/pkg/domain/error.go +++ b/pkg/domain/error.go @@ -15,22 +15,26 @@ const ( // Error codes as defined in the spec. const ( - ErrCodeInvalidAPIKey = "invalid_api_key" - ErrCodeInactiveAPIKey = "inactive_api_key" - ErrCodeInactiveAccount = "inactive_account" - ErrCodeUnsupportedModel = "unsupported_model" - ErrCodeUnsupportedEndpoint = "unsupported_endpoint" - ErrCodeUnsupportedFeature = "unsupported_feature" - ErrCodeContextLengthExceeded = "context_length_exceeded" - ErrCodeProviderUnavailable = "provider_unavailable" - ErrCodeProviderTimeout = "provider_timeout" - ErrCodeToolSchemaInvalid = "tool_schema_invalid" - ErrCodeToolMessageInvalid = "tool_message_invalid" - ErrCodeInvalidField = "invalid_field" - ErrCodeUnknownField = "unknown_field" - ErrCodeInsufficientBalance = "insufficient_balance" - ErrCodeClientCanceled = "client_canceled" - ErrCodeInternalError = "internal_error" + ErrCodeInvalidAPIKey = "invalid_api_key" + ErrCodeInactiveAPIKey = "inactive_api_key" + ErrCodeInactiveAccount = "inactive_account" + ErrCodeUnsupportedModel = "unsupported_model" + ErrCodeUnsupportedEndpoint = "unsupported_endpoint" + ErrCodeUnsupportedFeature = "unsupported_feature" + ErrCodeContextLengthExceeded = "context_length_exceeded" + ErrCodeProviderUnavailable = "provider_unavailable" + ErrCodeProviderRejectedRequest = "provider_rejected_request" + ErrCodeProviderConfiguration = "provider_configuration_error" + ErrCodeProviderRateLimited = "provider_rate_limited" + ErrCodeProviderError = "provider_error" + ErrCodeProviderTimeout = "provider_timeout" + ErrCodeToolSchemaInvalid = "tool_schema_invalid" + ErrCodeToolMessageInvalid = "tool_message_invalid" + ErrCodeInvalidField = "invalid_field" + ErrCodeUnknownField = "unknown_field" + ErrCodeInsufficientBalance = "insufficient_balance" + ErrCodeClientCanceled = "client_canceled" + ErrCodeInternalError = "internal_error" ) // GatewayError is the canonical error type for the gateway. @@ -39,6 +43,7 @@ type GatewayError struct { Type string Code string Message string + RetryAfter string // Validated provider retry guidance, forwarded as a header. // Metadata carries structured diagnostic fields for logging (never serialized to clients). Metadata map[string]any } @@ -122,7 +127,7 @@ func ErrInternal(msg string) *GatewayError { } func ErrProviderError(status int, msg string) *GatewayError { - return &GatewayError{HTTPStatus: status, Type: ErrTypeProvider, Code: ErrCodeProviderUnavailable, Message: msg} + return &GatewayError{HTTPStatus: status, Type: ErrTypeProvider, Code: ErrCodeProviderError, Message: msg} } // Control-plane error codes.