Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion router-tests/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ require (
github.com/docker/docker-credential-helpers v0.9.3 // indirect
github.com/docker/go-units v0.5.0 // indirect
github.com/dustin/go-humanize v1.0.1 // indirect
github.com/expr-lang/expr v1.17.3 // indirect
github.com/expr-lang/expr v1.17.6 // indirect
Comment thread
SkArchon marked this conversation as resolved.
github.com/fatih/color v1.18.0 // indirect
github.com/felixge/httpsnoop v1.0.4 // indirect
github.com/go-chi/chi/v5 v5.2.2 // indirect
Expand Down
1 change: 1 addition & 0 deletions router-tests/go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ github.com/dustin/go-humanize v1.0.1 h1:GzkhY7T5VNhEkwH0PVJgjz+fX1rhBrR7pRT3mDkp
github.com/dustin/go-humanize v1.0.1/go.mod h1:Mu1zIs6XwVuF/gI1OepvI0qD18qycQx+mFykh5fBlto=
github.com/expr-lang/expr v1.17.3 h1:myeTTuDFz7k6eFe/JPlep/UsiIjVhG61FMHFu63U7j0=
github.com/expr-lang/expr v1.17.3/go.mod h1:8/vRC7+7HBzESEqt5kKpYXxrxkr31SaO8r40VO/1IT4=
github.com/expr-lang/expr v1.17.6/go.mod h1:8/vRC7+7HBzESEqt5kKpYXxrxkr31SaO8r40VO/1IT4=
github.com/fatih/color v1.13.0/go.mod h1:kLAiJbzzSOZDVNGyDpeOxJ47H46qBXwg5ILebYFFOfk=
github.com/fatih/color v1.18.0 h1:S8gINlzdQ840/4pfAwic/ZE0djQEH3wM94VfqLTZcOM=
github.com/fatih/color v1.18.0/go.mod h1:4FelSpRwEGDpQ12mAdzqdOukCy4u8WUtOY6lkT/6HfU=
Expand Down
35 changes: 21 additions & 14 deletions router/core/graph_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -1156,6 +1156,21 @@ func (s *graphServer) buildGraphMux(
baseConnMetricStore = s.connectionMetrics
}

// Build retry options and handle any expression compilation errors
shouldRetryFunc, err := BuildRetryFunction(s.retryOptions.Expression, s.logger)
if err != nil {
return nil, fmt.Errorf("failed to build retry function: %w", err)
}

retryOptions := retrytransport.RetryOptions{
Enabled: s.retryOptions.Enabled,
MaxRetryCount: s.retryOptions.MaxRetryCount,
MaxDuration: s.retryOptions.MaxDuration,
Interval: s.retryOptions.Interval,
Expression: s.retryOptions.Expression,
ShouldRetry: shouldRetryFunc,
}

Comment thread
SkArchon marked this conversation as resolved.
Comment thread
SkArchon marked this conversation as resolved.
ecb := &ExecutorConfigurationBuilder{
introspection: s.introspection,
baseURL: s.baseURL,
Expand All @@ -1171,20 +1186,12 @@ func (s *graphServer) buildGraphMux(
FrameTimeout: s.engineExecutionConfiguration.WebSocketClientFrameTimeout,
},
transportOptions: &TransportOptions{
SubgraphTransportOptions: s.subgraphTransportOptions,
PreHandlers: s.preOriginHandlers,
PostHandlers: s.postOriginHandlers,
MetricStore: gm.metricStore,
ConnectionMetricStore: baseConnMetricStore,
RetryOptions: retrytransport.RetryOptions{
Enabled: s.retryOptions.Enabled,
MaxRetryCount: s.retryOptions.MaxRetryCount,
MaxDuration: s.retryOptions.MaxDuration,
Interval: s.retryOptions.Interval,
ShouldRetry: func(err error, req *http.Request, resp *http.Response) bool {
return retrytransport.IsRetryableError(err, resp) && !isMutationRequest(req.Context())
},
},
SubgraphTransportOptions: s.subgraphTransportOptions,
PreHandlers: s.preOriginHandlers,
PostHandlers: s.postOriginHandlers,
MetricStore: gm.metricStore,
ConnectionMetricStore: baseConnMetricStore,
RetryOptions: retryOptions,
TracerProvider: s.tracerProvider,
TracePropagators: s.compositePropagator,
LocalhostFallbackInsideDocker: s.localhostFallbackInsideDocker,
Expand Down
50 changes: 50 additions & 0 deletions router/core/retry_expression_builder.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
package core

import (
"fmt"
"net/http"

"github.com/wundergraph/cosmo/router/internal/expr"
"github.com/wundergraph/cosmo/router/internal/retrytransport"
"go.uber.org/zap"
)
Comment thread
SkArchon marked this conversation as resolved.

const DefaultRetryExpression = "IsRetryableStatusCode() || IsConnectionError() || IsTimeout()"
Comment thread
Noroth marked this conversation as resolved.
Outdated

// BuildRetryFunction creates a ShouldRetry function based on the provided expression
func BuildRetryFunction(expression string, logger *zap.Logger) (retrytransport.ShouldRetryFunc, error) {
// Use default expression if empty string is passed
if expression == "" {
expression = DefaultRetryExpression
}

// Create the retry expression manager
manager, err := expr.NewRetryExpressionManager(expression)
if err != nil {
return nil, fmt.Errorf("failed to compile retry expression: %w", err)
Comment thread
Noroth marked this conversation as resolved.
Outdated
}

// Return expression-based retry function
return func(err error, req *http.Request, resp *http.Response) bool {
// Never retry mutations, regardless of expression result
if isMutationRequest(req.Context()) {
return false
}

// Create retry context
ctx := expr.LoadRetryContext(err, resp)

// Evaluate the expression
shouldRetry, evalErr := manager.ShouldRetry(ctx)
if evalErr != nil {
logger.Error("Failed to evaluate retry expression",
zap.Error(evalErr),
zap.String("expression", expression),
)
// Disable retries on evaluation error
return false
}

return shouldRetry
}, nil
}
154 changes: 154 additions & 0 deletions router/core/retry_expression_builder_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,154 @@
package core

import (
"errors"
"net/http"
"syscall"
"testing"

"github.com/stretchr/testify/assert"
"go.uber.org/zap"
)

func TestBuildRetryFunction(t *testing.T) {
logger := zap.NewNop()

t.Run("default expression behavior", func(t *testing.T) {
// Use the default expression that would be in the config
fn, err := BuildRetryFunction(DefaultRetryExpression, logger)
assert.NoError(t, err)
assert.NotNil(t, fn)

// Test default behavior - should retry on 500
req, _ := http.NewRequest("GET", "http://example.com", nil)
resp := &http.Response{StatusCode: 500}
assert.True(t, fn(nil, req, resp))

// Should not retry on 200
resp.StatusCode = 200
assert.False(t, fn(nil, req, resp))

// Note: Testing mutation behavior would require setting up a proper request context
// which is beyond the scope of this unit test. The mutation check is tested
// in integration tests.

// Test with errors - only expression-defined errors are handled here
req, _ = http.NewRequest("GET", "http://example.com", nil)
assert.True(t, fn(syscall.ETIMEDOUT, req, nil))
assert.True(t, fn(errors.New("connection refused"), req, nil))
assert.False(t, fn(errors.New("unexpected EOF"), req, nil)) // EOF is now handled at transport layer, not expression
assert.False(t, fn(errors.New("some other error"), req, nil))
})

t.Run("expression-based retry", func(t *testing.T) {
expression := "statusCode == 500 || statusCode == 503"
fn, err := BuildRetryFunction(expression, logger)
assert.NoError(t, err)
assert.NotNil(t, fn)

req, _ := http.NewRequest("GET", "http://example.com", nil)

// Should retry on 500
resp := &http.Response{StatusCode: 500}
assert.True(t, fn(nil, req, resp))

// Should retry on 503
resp.StatusCode = 503
assert.True(t, fn(nil, req, resp))

// Should not retry on 502
resp.StatusCode = 502
assert.False(t, fn(nil, req, resp))
})

t.Run("expression with error conditions", func(t *testing.T) {
expression := "IsTimeout() || statusCode == 503"
fn, err := BuildRetryFunction(expression, logger)
assert.NoError(t, err)
assert.NotNil(t, fn)

req, _ := http.NewRequest("GET", "http://example.com", nil)

// Should retry on timeout error
err = syscall.ETIMEDOUT
assert.True(t, fn(err, req, nil))

// Should retry on 503
resp := &http.Response{StatusCode: 503}
assert.True(t, fn(nil, req, resp))

// Should not retry on other errors
err = errors.New("some other error")
assert.False(t, fn(err, req, nil))
})

t.Run("invalid expression returns error", func(t *testing.T) {
expression := "invalid syntax +++"
fn, err := BuildRetryFunction(expression, logger)
assert.Error(t, err)
assert.Nil(t, fn)
assert.Contains(t, err.Error(), "failed to compile retry expression")
})

t.Run("empty expression uses default", func(t *testing.T) {
fn, err := BuildRetryFunction("", logger)
assert.NoError(t, err)
assert.NotNil(t, fn)

// Test with retryable status code
req, _ := http.NewRequest("GET", "http://example.com", nil)
resp := &http.Response{StatusCode: 502}
assert.True(t, fn(nil, req, resp))

// Test with connection error
err = errors.New("connection refused")
assert.True(t, fn(err, req, nil))

// Test with timeout error
err = syscall.ETIMEDOUT
assert.True(t, fn(err, req, nil))

// Test with non-retryable error
err = errors.New("some other error")
assert.False(t, fn(err, req, nil))
})

t.Run("expression that always returns true", func(t *testing.T) {
expression := "true" // Always retry
fn, err := BuildRetryFunction(expression, logger)
assert.NoError(t, err)
assert.NotNil(t, fn)

req, _ := http.NewRequest("GET", "http://example.com", nil)
resp := &http.Response{StatusCode: 500}

// Should retry when expression is true
assert.True(t, fn(nil, req, resp))

// Even for status codes that wouldn't normally retry
resp.StatusCode = 200
assert.True(t, fn(nil, req, resp))
})

t.Run("complex expression", func(t *testing.T) {
expression := "(statusCode >= 500 && statusCode < 600) || IsConnectionError()"
fn, err := BuildRetryFunction(expression, logger)
assert.NoError(t, err)
assert.NotNil(t, fn)

req, _ := http.NewRequest("GET", "http://example.com", nil)

// Test 5xx errors
resp := &http.Response{StatusCode: 503}
assert.True(t, fn(nil, req, resp))

// Test connection error
err = errors.New("connection refused")
assert.True(t, fn(err, req, nil))

// Test non-matching conditions
resp.StatusCode = 404
err = errors.New("some other error")
assert.False(t, fn(err, req, resp))
})
}
3 changes: 2 additions & 1 deletion router/core/router.go
Original file line number Diff line number Diff line change
Expand Up @@ -1749,13 +1749,14 @@ func WithSubgraphCircuitBreakerOptions(opts *SubgraphCircuitBreakerOptions) Opti
}
}

func WithSubgraphRetryOptions(enabled bool, maxRetryCount int, retryMaxDuration, retryInterval time.Duration) Option {
func WithSubgraphRetryOptions(enabled bool, maxRetryCount int, retryMaxDuration, retryInterval time.Duration, expression string) Option {
return func(r *Router) {
r.retryOptions = retrytransport.RetryOptions{
Enabled: enabled,
MaxRetryCount: maxRetryCount,
MaxDuration: retryMaxDuration,
Interval: retryInterval,
Expression: expression,
}
}
Comment thread
SkArchon marked this conversation as resolved.
Outdated
}
Comment thread
SkArchon marked this conversation as resolved.
Comment thread
SkArchon marked this conversation as resolved.
Expand Down
1 change: 1 addition & 0 deletions router/core/supervisor_instance.go
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,7 @@ func optionsFromResources(logger *zap.Logger, config *config.Config) []Option {
config.TrafficShaping.All.BackoffJitterRetry.MaxAttempts,
config.TrafficShaping.All.BackoffJitterRetry.MaxDuration,
config.TrafficShaping.All.BackoffJitterRetry.Interval,
config.TrafficShaping.All.BackoffJitterRetry.Expression,
),
Comment thread
SkArchon marked this conversation as resolved.
WithCors(&cors.Config{
Enabled: config.CORS.Enabled,
Expand Down
2 changes: 1 addition & 1 deletion router/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ require (
github.com/caarlos0/env/v11 v11.3.1
github.com/cep21/circuit/v4 v4.0.0
github.com/dgraph-io/ristretto/v2 v2.1.0
github.com/expr-lang/expr v1.17.3
github.com/expr-lang/expr v1.17.6
github.com/goccy/go-json v0.10.3
github.com/google/go-containerregistry v0.20.3
github.com/google/uuid v1.6.0
Expand Down
4 changes: 2 additions & 2 deletions router/go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,8 @@ github.com/docker/go-units v0.5.0 h1:69rxXcBk27SvSaaxTtLh/8llcHD8vYHT7WSdRZ/jvr4
github.com/docker/go-units v0.5.0/go.mod h1:fgPhTUdO+D/Jk86RDLlptpiXQzgHJF7gydDDbaIK4Dk=
github.com/dustin/go-humanize v1.0.1 h1:GzkhY7T5VNhEkwH0PVJgjz+fX1rhBrR7pRT3mDkpeCY=
github.com/dustin/go-humanize v1.0.1/go.mod h1:Mu1zIs6XwVuF/gI1OepvI0qD18qycQx+mFykh5fBlto=
github.com/expr-lang/expr v1.17.3 h1:myeTTuDFz7k6eFe/JPlep/UsiIjVhG61FMHFu63U7j0=
github.com/expr-lang/expr v1.17.3/go.mod h1:8/vRC7+7HBzESEqt5kKpYXxrxkr31SaO8r40VO/1IT4=
github.com/expr-lang/expr v1.17.6 h1:1h6i8ONk9cexhDmowO/A64VPxHScu7qfSl2k8OlINec=
github.com/expr-lang/expr v1.17.6/go.mod h1:8/vRC7+7HBzESEqt5kKpYXxrxkr31SaO8r40VO/1IT4=
github.com/fatih/color v1.13.0/go.mod h1:kLAiJbzzSOZDVNGyDpeOxJ47H46qBXwg5ILebYFFOfk=
github.com/fatih/color v1.18.0 h1:S8gINlzdQ840/4pfAwic/ZE0djQEH3wM94VfqLTZcOM=
github.com/fatih/color v1.18.0/go.mod h1:4FelSpRwEGDpQ12mAdzqdOukCy4u8WUtOY6lkT/6HfU=
Expand Down
Loading
Loading