plans, ratelimit: a sponsorship or ceiling never tightens the backstop (#358)
For the three rate fields zero on the plan side means no ceiling, but generous took the numeric max, so an unlimited plan with a sponsored rate_per_second of 1 resolved to a ceiling of 1, below the platform default the backstop promises. generousRate keeps the plan's zero (it is already the most generous value) and otherwise takes the larger; a sponsored zero stays unset, as for every other field. Defence in depth, spec §9 ruling 1: Resolve raises a non-zero Rate or Burst ceiling below the platform default to the default, so no route to the edge can push the backstop under it. ClientShare has no floor. TestRateCeilingClampsAtApply used a 2/2 backstop ceiling, which the floor now raises; it shows the per-request clamp with a share ceiling instead.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
5 files changed,
+92
−16
internal/edge/ratelimit_test.go+15 −11internal/plans/catalog.go+14 −3internal/plans/catalog_ratelimit_test.go+29 −0internal/ratelimit/settings.go+12 −0internal/ratelimit/settings_test.go+22 −2
diff --git a/internal/edge/ratelimit_test.go b/internal/edge/ratelimit_test.go| index da99aebea..c31e83bfa 100644 |
| --- a/internal/edge/ratelimit_test.go |
| +++ b/internal/edge/ratelimit_test.go |
| @@ -718,27 +718,31 @@ func TestRateHopKeyedByClient(t *testing.T) { |
| // ---- settings --------------------------------------------------------------- |
| // Review Focus 4: the plan ceiling is applied per request, over whatever the |
| -// owner set. |
| +// owner set. The owner allows each client 16 in-flight while busy; a ceiling |
| +// of 1 sheds the client's second request behind its first. |
| func TestRateCeilingClampsAtApply(t *testing.T) { |
| - ts, _ := countingUpstream(t, "ok", nil) |
| - s := newRateScene(t, "enforce", nil, rlRoute(ts.Listener.Addr().String(), `{"rate":1000,"burst":1000}`)) |
| + up := newBlockingUpstream(t) |
| + s := newRateScene(t, "enforce", nil, rlRoute(up.addr(), `{"busy_at":1,"client_share":16}`)) |
| + t.Cleanup(up.open) // before the front server closes on a failure |
| var seen atomic.Value |
| s.e.SetRateCeilingFor(func(rt registry.Route) ratelimit.Ceiling { |
| seen.Store(rt.Host) |
| - return ratelimit.Ceiling{Rate: 2, Burst: 2} |
| + return ratelimit.Ceiling{ClientShare: 1} |
| }) |
| - wantOK(t, s.get(t, "203.0.113.1", rlHost, "/"), "first") |
| - wantOK(t, s.get(t, "203.0.113.1", rlHost, "/"), "second") |
| - want429(t, s.get(t, "203.0.113.1", rlHost, "/"), "1", "third within a second, over the ceiling") |
| + first := s.goGet("203.0.113.1", rlHost, "/") |
| + up.waitArrived(t, "the first request") |
| + want429(t, s.get(t, "203.0.113.1", rlHost, "/"), "1", "the second while busy, over the ceiling's share") |
| if got, _ := seen.Load().(string); got != rlHost { |
| t.Fatalf("ceiling hook saw host %q, want %q", got, rlHost) |
| } |
| - // No hook is no ceiling: the owner's 1000 applies again. |
| + // No hook is no ceiling: the owner's 16 applies again. |
| s.e.SetRateCeilingFor(nil) |
| - for i := range 5 { |
| - wantOK(t, s.get(t, "203.0.113.2", rlHost, "/"), fmt.Sprintf("request %d without a ceiling", i)) |
| - } |
| + second := s.goGet("203.0.113.1", rlHost, "/") |
| + up.waitArrived(t, "the second request without a ceiling") |
| + up.open() |
| + wantOK(t, waitResult(t, first, "first"), "first") |
| + wantOK(t, waitResult(t, second, "second"), "second without a ceiling") |
| } |
| // Review Focus 5: a column the edge cannot use costs the instance its own |
diff --git a/internal/plans/catalog.go b/internal/plans/catalog.go| index 28441afa1..20bd4ae5d 100644 |
| --- a/internal/plans/catalog.go |
| +++ b/internal/plans/catalog.go |
| @@ -302,9 +302,9 @@ func (l Limits) generous(o Limits) Limits { |
| l.EmailPerDay = max(l.EmailPerDay, o.EmailPerDay) |
| l.BandwidthBytesMonth = max(l.BandwidthBytesMonth, o.BandwidthBytesMonth) |
| l.HistoryDays = max(l.HistoryDays, o.HistoryDays) |
| - l.RateClientShare = max(l.RateClientShare, o.RateClientShare) |
| - l.RatePerSecond = max(l.RatePerSecond, o.RatePerSecond) |
| - l.RateBurst = max(l.RateBurst, o.RateBurst) |
| + l.RateClientShare = generousRate(l.RateClientShare, o.RateClientShare) |
| + l.RatePerSecond = generousRate(l.RatePerSecond, o.RatePerSecond) |
| + l.RateBurst = generousRate(l.RateBurst, o.RateBurst) |
| l.StorageSelfServe = l.StorageSelfServe || o.StorageSelfServe |
| l.CustomDomains = l.CustomDomains || o.CustomDomains |
| l.Fleets = l.Fleets || o.Fleets |
| @@ -318,6 +318,17 @@ func (l Limits) generous(o Limits) Limits { |
| return l |
| } |
| +// generousRate is generous for a rate field. On the plan side zero is no |
| +// ceiling, the most generous value there is, so it stays zero whatever is |
| +// sponsored; plain max would turn an unlimited plan and a sponsored 1 into |
| +// 1. A sponsored zero is unset, as for every other field. |
| +func generousRate(plan, sponsored int) int { |
| + if plan == 0 || sponsored == 0 { |
| + return plan |
| + } |
| + return max(plan, sponsored) |
| +} |
| + |
| // addPack is v + qty*add, refusing (ok = false) anything that would overflow |
| // int64. Values are non-negative (validate), so only the upper bound matters; |
| // int is 64-bit on every platform this builds for. |
diff --git a/internal/plans/catalog_ratelimit_test.go b/internal/plans/catalog_ratelimit_test.go| index 91525276c..73059be34 100644 |
| --- a/internal/plans/catalog_ratelimit_test.go |
| +++ b/internal/plans/catalog_ratelimit_test.go |
| @@ -154,3 +154,32 @@ func TestEffectiveRateZeroUnderCeilingTakesTheCeiling(t *testing.T) { |
| t.Fatalf("no plan value and no ceiling = %+v, want zero", got.RateCeiling()) |
| } |
| } |
| + |
| +// A rate field of zero on the plan side is unlimited, so a sponsored value, |
| +// however large, can only be stricter: the result stays unlimited. Plan 0 |
| +// and sponsored 1 is not 1. |
| +func TestEffectiveSponsoredKeepsUnlimitedRate(t *testing.T) { |
| + c := exampleCatalog(t) |
| + if c.Ceilings.RateClientShare != 0 || c.Ceilings.RatePerSecond != 0 || c.Ceilings.RateBurst != 0 { |
| + t.Fatalf("example catalog has rate ceilings %+v; this test needs none", c.Ceilings.RateCeiling()) |
| + } |
| + got, err := c.Effective("free", nil, &Limits{RateClientShare: 1, RatePerSecond: 1, RateBurst: 1}) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if got.RateCeiling() != (ratelimit.Ceiling{}) { |
| + t.Fatalf("unlimited plan with sponsored 1s = %+v, want unlimited (zero)", got.RateCeiling()) |
| + } |
| + |
| + // One field unlimited, the others set: only the unlimited one stays zero. |
| + p := c.Plans["personal"] |
| + p.Limits.RateClientShare, p.Limits.RatePerSecond, p.Limits.RateBurst = 20, 0, 1500 |
| + c.Plans["personal"] = p |
| + got, err = c.Effective("personal", nil, &Limits{RateClientShare: 40, RatePerSecond: 400, RateBurst: 1200}) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if want := (ratelimit.Ceiling{ClientShare: 40, Rate: 0, Burst: 1500}); got.RateCeiling() != want { |
| + t.Fatalf("mixed = %+v, want %+v", got.RateCeiling(), want) |
| + } |
| +} |
diff --git a/internal/ratelimit/settings.go b/internal/ratelimit/settings.go| index 4ae20f3b3..45abb4fba 100644 |
| --- a/internal/ratelimit/settings.go |
| +++ b/internal/ratelimit/settings.go |
| @@ -145,7 +145,19 @@ type Effective struct { |
| // Resolve starts from Defaults, overlays the owner's non-zero fields, lets |
| // each path rule inherit any field it left zero from the resolved base, and |
| // then clamps ClientShare, Rate and Burst to the ceiling's non-zero values. |
| +// |
| +// A plan can never push the backstop below the platform default (spec §9, |
| +// ruling 1). The catalog refuses such a ceiling when it loads; a non-zero |
| +// Rate or Burst ceiling under the default that arrives here anyway, by |
| +// another route, is raised to the default rather than trusted. ClientShare |
| +// has no such floor. |
| func Resolve(s Settings, c Ceiling) Effective { |
| + if c.Rate != 0 { |
| + c.Rate = max(c.Rate, Defaults.Rate) |
| + } |
| + if c.Burst != 0 { |
| + c.Burst = max(c.Burst, Defaults.Burst) |
| + } |
| e := Effective{BusyAt: Defaults.BusyAt, Base: Defaults.Rule} |
| if s.BusyAt != 0 { |
| e.BusyAt = s.BusyAt |
diff --git a/internal/ratelimit/settings_test.go b/internal/ratelimit/settings_test.go| index 02305aef5..655c30d17 100644 |
| --- a/internal/ratelimit/settings_test.go |
| +++ b/internal/ratelimit/settings_test.go |
| @@ -141,12 +141,32 @@ func TestResolveClampsToCeiling(t *testing.T) { |
| } |
| // A ceiling also catches the defaults, and clamps the other two fields. |
| - e = Resolve(Settings{}, Ceiling{ClientShare: 4, Burst: 100}) |
| - if e.Base.ClientShare != 4 || e.Base.Burst != 100 || e.Base.Rate != 300 { |
| + e = Resolve(Settings{}, Ceiling{ClientShare: 4, Burst: 1000}) |
| + if e.Base.ClientShare != 4 || e.Base.Burst != 1000 || e.Base.Rate != 300 { |
| t.Fatalf("base = %+v", e.Base) |
| } |
| } |
| +// A ceiling never pushes the backstop below the platform default (§9 ruling |
| +// 1): a rate or burst ceiling under the default is raised to it. The owner |
| +// may still set their own lower values, and the share ceiling has no floor. |
| +func TestResolveCeilingNeverBelowDefaults(t *testing.T) { |
| + s := Settings{ |
| + Rule: Rule{ClientShare: 32, Rate: 5000, Burst: 9000}, |
| + Paths: []PathRule{{Prefix: "/api/", Rule: Rule{Rate: 8000}}}, |
| + } |
| + e := Resolve(s, Ceiling{ClientShare: 4, Rate: 1, Burst: 1}) |
| + want := Rule{ClientShare: 4, Rate: Defaults.Rate, Burst: Defaults.Burst} |
| + if e.Base != want || e.Paths[0].Rule != want { |
| + t.Fatalf("base %+v, /api/ %+v, want %+v for both", e.Base, e.Paths[0].Rule, want) |
| + } |
| + |
| + e = Resolve(Settings{Rule: Rule{Rate: 10, Burst: 20}}, Ceiling{Rate: 1, Burst: 1}) |
| + if e.Base.Rate != 10 || e.Base.Burst != 20 { |
| + t.Fatalf("owner's own lower values = %+v, want rate 10 burst 20", e.Base) |
| + } |
| +} |
| + |
| func TestRuleForLongestPrefix(t *testing.T) { |
| s := Settings{Paths: []PathRule{ |
| {Prefix: "/api/", Rule: Rule{Rate: 10}}, |