implement last-known-good fallback for dynamic limit settings

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) {

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")
+		})
+	}
+}

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 {

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)
 }
 

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) {

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)