diff --git a/pkg/settings/limits/bound.go b/pkg/settings/limits/bound.go index a9e2670ce8..b566ce0d6d 100644 --- a/pkg/settings/limits/bound.go +++ b/pkg/settings/limits/bound.go @@ -206,21 +206,14 @@ func (b *boundLimiter[N]) Check(ctx context.Context, amount N) error { } func (b *boundLimiter[N]) Limit(ctx context.Context) (N, error) { - var zero N if err := b.wg.TryAdd(1); err != nil { + var zero N return zero, err } defer b.wg.Done() - tenant, bound, err := b.get(ctx) - if err != nil { - return zero, err - } - if tenant == "" && b.scope != settings.ScopeGlobal { - return zero, nil // fail open - } - - return bound, nil + _, bound, err := b.get(ctx) + return bound, err // bound is get()'s resolved value; zero if no tenant, or default on error } func (b *boundLimiter[N]) get(ctx context.Context) (tenant string, bound N, err error) { diff --git a/pkg/settings/limits/default_on_error_test.go b/pkg/settings/limits/default_on_error_test.go new file mode 100644 index 0000000000..f98f5d6c45 --- /dev/null +++ b/pkg/settings/limits/default_on_error_test.go @@ -0,0 +1,150 @@ +package limits + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/smartcontractkit/chainlink-common/pkg/config" + "github.com/smartcontractkit/chainlink-common/pkg/settings" +) + +var errGetterUnavailable = errors.New("settings getter unavailable") + +// failingGetter always fails GetScoped, standing in for a settings-service outage. +type failingGetter struct{} + +func (failingGetter) GetScoped(context.Context, settings.Scope, string) (string, error) { + return "", errGetterUnavailable +} + +// TestLimiter_Limit_ReturnsDefaultOnReadFailure is the parity fix: Limit() must return +// the value get() already resolved (the compiled default, on a read failure) alongside +// the error, instead of discarding it. +func TestLimiter_Limit_ReturnsDefaultOnReadFailure(t *testing.T) { + t.Parallel() + + t.Run("bound", func(t *testing.T) { + t.Parallel() + setting := settings.Size(1 * config.GByte) + setting.Key, setting.Scope = "test.bound", settings.ScopeGlobal + bl, err := MakeUpperBoundLimiter(Factory{Settings: failingGetter{}}, setting) + require.NoError(t, err) + t.Cleanup(func() { assert.NoError(t, bl.Close()) }) + + v, err := bl.Limit(t.Context()) + require.ErrorIs(t, err, errGetterUnavailable) + assert.Equal(t, 1*config.GByte, v) + }) + + t.Run("time", func(t *testing.T) { + t.Parallel() + setting := settings.Duration(1 * time.Minute) + setting.Key, setting.Scope = "test.time", settings.ScopeGlobal + tl, err := Factory{Settings: failingGetter{}}.MakeTimeLimiter(setting) + require.NoError(t, err) + t.Cleanup(func() { assert.NoError(t, tl.Close()) }) + + d, err := tl.Limit(t.Context()) + require.ErrorIs(t, err, errGetterUnavailable) + assert.Equal(t, 1*time.Minute, d) + }) + + t.Run("gate", func(t *testing.T) { + t.Parallel() + setting := settings.Bool(true) + setting.Key, setting.Scope = "test.gate", settings.ScopeGlobal + gl, err := MakeGateLimiter(Factory{Settings: failingGetter{}}, setting) + require.NoError(t, err) + t.Cleanup(func() { assert.NoError(t, gl.Close()) }) + + open, err := gl.Limit(t.Context()) + require.ErrorIs(t, err, errGetterUnavailable) + assert.True(t, open) + }) + + t.Run("range", func(t *testing.T) { + t.Parallel() + setting := settings.NewSetting(settings.Range[int]{Lower: 1, Upper: 5}, settings.ParseRangeFn(func(s string) (int, error) { + return 0, nil + })) + setting.Key, setting.Scope = "test.range", settings.ScopeGlobal + rl, err := MakeRangeLimiter[int](Factory{Settings: failingGetter{}}, setting) + require.NoError(t, err) + t.Cleanup(func() { assert.NoError(t, rl.Close()) }) + + got, err := rl.Limit(t.Context()) + require.ErrorIs(t, err, errGetterUnavailable) + assert.Equal(t, settings.Range[int]{Lower: 1, Upper: 5}, got) + }) +} + +// TestTimeLimiter_WithTimeout_UsableOnReadFailure is the fix for the actual production +// incident: WithTimeout used to return (nil, nil, err) on a read failure, causing callers +// to drop the unit of work instead of running it with the compiled default timeout. +func TestTimeLimiter_WithTimeout_UsableOnReadFailure(t *testing.T) { + t.Parallel() + + setting := settings.Duration(10 * time.Second) + setting.Key, setting.Scope = "test.time.with-timeout", settings.ScopeGlobal + tl, err := Factory{Settings: failingGetter{}}.MakeTimeLimiter(setting) + require.NoError(t, err) + t.Cleanup(func() { assert.NoError(t, tl.Close()) }) + + before := time.Now() + ctx, done, withTimeoutErr := tl.WithTimeout(t.Context()) + require.Error(t, withTimeoutErr, "err is advisory, not fatal - the read did fail") + require.NotNil(t, ctx, "ctx must be usable despite the read failure") + require.NotNil(t, done) + defer done() + + deadline, ok := ctx.Deadline() + require.True(t, ok, "ctx must carry a real deadline, not be unbounded") + assert.InDelta(t, 10*time.Second, deadline.Sub(before), float64(2*time.Second), + "deadline should be based on the compiled default (10s), not hang open or fire instantly") +} + +// TestTimeLimiter_WithTimeout_UsableWithoutTenant covers the missing-tenant path. The timeout +// doesn't depend on the tenant, so the context must still be bounded by the compiled default: +// a zero timeout would expire immediately, and a nil context would make callers drop the work. +// A tenant that was required but missing still surfaces an error, so the bug stays visible. +func TestTimeLimiter_WithTimeout_UsableWithoutTenant(t *testing.T) { + t.Parallel() + + for _, tt := range []struct { + scope settings.Scope + expectErr bool + }{ + {settings.ScopeOrg, false}, // tenant not required + {settings.ScopeWorkflow, true}, // tenant required + } { + t.Run(tt.scope.String(), func(t *testing.T) { + t.Parallel() + setting := settings.Duration(10 * time.Second) + setting.Key, setting.Scope = "test.time.no-tenant", tt.scope + tl, err := Factory{}.MakeTimeLimiter(setting) + require.NoError(t, err) + t.Cleanup(func() { assert.NoError(t, tl.Close()) }) + + before := time.Now() + ctx, done, err := tl.WithTimeout(t.Context()) // no contexts.WithCRE, so no tenant + if tt.expectErr { + require.Error(t, err, "a required but missing tenant must stay visible") + } else { + require.NoError(t, err) + } + require.NotNil(t, ctx, "ctx must be usable without a tenant") + require.NotNil(t, done) + defer done() + + deadline, ok := ctx.Deadline() + require.True(t, ok, "ctx must carry a real deadline, not be unbounded") + assert.InDelta(t, 10*time.Second, deadline.Sub(before), float64(2*time.Second), + "deadline should be based on the compiled default (10s), not fire instantly") + }) + } +} diff --git a/pkg/settings/limits/gate.go b/pkg/settings/limits/gate.go index 611362ebf6..51a7e9a683 100644 --- a/pkg/settings/limits/gate.go +++ b/pkg/settings/limits/gate.go @@ -175,11 +175,7 @@ func (g *gateLimiter) Limit(ctx context.Context) (bool, error) { defer g.wg.Done() _, limit, err := g.get(ctx) - if err != nil { - return false, err - } - - return limit, nil + return limit, err // limit is get()'s resolved value; false if no tenant, or default on error } func (g *gateLimiter) AllowErr(ctx context.Context) error { diff --git a/pkg/settings/limits/limits.go b/pkg/settings/limits/limits.go index ccc1e22a99..ffd7c9bb21 100644 --- a/pkg/settings/limits/limits.go +++ b/pkg/settings/limits/limits.go @@ -29,7 +29,7 @@ type Number interface { type Limiter[N any] interface { io.Closer // Limiters spawn background goroutines and must be closed. - // Limit returns the current limit. + // Limit returns the current limit, or an error along with a usable fallback value. Limit(context.Context) (N, error) } diff --git a/pkg/settings/limits/range.go b/pkg/settings/limits/range.go index 136ff31f65..550111a9ee 100644 --- a/pkg/settings/limits/range.go +++ b/pkg/settings/limits/range.go @@ -195,21 +195,14 @@ func (b *rangeLimiter[N]) Check(ctx context.Context, amount N) error { } func (b *rangeLimiter[N]) Limit(ctx context.Context) (settings.Range[N], error) { - var zero settings.Range[N] if err := b.wg.TryAdd(1); err != nil { + var zero settings.Range[N] return zero, err } defer b.wg.Done() - tenant, bound, err := b.get(ctx) - if err != nil { - return zero, err - } - if tenant == "" && b.scope != settings.ScopeGlobal { - return zero, nil // fail open - } - - return bound, nil + _, bound, err := b.get(ctx) + return bound, err // bound is get()'s resolved value; zero if no tenant, or default on error } func (b *rangeLimiter[N]) get(ctx context.Context) (tenant string, bound settings.Range[N], err error) { diff --git a/pkg/settings/limits/time.go b/pkg/settings/limits/time.go index ce2e23b477..9a9fa5ab8b 100644 --- a/pkg/settings/limits/time.go +++ b/pkg/settings/limits/time.go @@ -191,12 +191,6 @@ func (l *timeLimiter) WithTimeout(ctx context.Context) (context.Context, func(), defer l.wg.Done() tenant, timeout, err := l.get(ctx) - if err != nil { - return nil, nil, err - } - if tenant == "" && l.scope != settings.ScopeGlobal { - return ctx, func() {}, nil // fail open - } countTimeout := func() { l.countTimeout(ctx) } // constructing this first to reference the original ctx ctx, cancel := context.WithTimeoutCause(ctx, timeout, ErrorTimeLimited{Key: l.key, Scope: l.scope, Tenant: tenant, Timeout: timeout}) @@ -211,33 +205,29 @@ func (l *timeLimiter) WithTimeout(ctx context.Context) (context.Context, func(), l.countSuccess(ctx) } cancel() - }, nil + }, err // timeout is get()'s resolved value, or the compiled default; err is advisory } func (l *timeLimiter) Limit(ctx context.Context) (time.Duration, error) { if err := l.wg.TryAdd(1); err != nil { - return -1, err + return 0, err } defer l.wg.Done() - tenant, timeout, err := l.get(ctx) - if err != nil { - return -1, err - } - if tenant == "" && l.scope != settings.ScopeGlobal { - return -1, nil // fail open - } - - return timeout, nil + _, timeout, err := l.get(ctx) + return timeout, err // timeout is get()'s resolved value, or the compiled default; err is advisory } func (l *timeLimiter) get(ctx context.Context) (tenant string, timeout time.Duration, err error) { if l.scope != settings.ScopeGlobal { tenant = l.scope.Value(ctx) if tenant == "" { + // The timeout doesn't depend on the tenant, so fall back to the compiled default + // rather than leaving it at zero, which would mean an already-expired context. + timeout = l.defaultTimeout if !l.scope.IsTenantRequired() { kvs := contexts.CREValue(ctx).LoggerKVs() - l.lggr.Errorw("Unable to get scoped time limit due to missing tenant: failing open", append([]any{"scope", l.scope}, kvs...)...) + l.lggr.Errorw("Unable to get scoped time limit due to missing tenant: using default value", append([]any{"scope", l.scope, "default", timeout}, kvs...)...) return } err = fmt.Errorf("unable to get scoped time limit due to missing tenant for scope: %s", l.scope)