fix(rate_limit): validate static limit and period at construction

Sankalprequested to merge
sankalp/rate-limit-validate-limit-period into master

Description

Rule.new now raises ArgumentError for a static limit under 1 and a static period under one second on :limit and :log rules.

Both values were required keywords but never validated. A limit: 0 rule constructed fine and then blocked every matching request from the first one (RateLimit-Limit: 0, RateLimit-Remaining: 0). A zero or sub-second period reached EXPIRE as 0, which deleted the counter on every write. ban_for already has this check; this finishes the same job.

Matches the Go SDK, which rejects a static Limit under 1 and a static Period under one second at construction since gitlab-org/labkit!668 (merged).

Scope:

  • :skip rules are exempt, since they count nothing.
  • Callables are not validated. A callable limit resolving to 0 stays the supported way to express "deny all" dynamically; the README now says so.
  • Validation is unconditional, like normalize_ban_for and unlike the dev-or-test-gated name format check. A production config with a static limit: 0 now fails at boot instead of silently denying all traffic.
  • Following the normalize_ban_for shape, numeric strings such as limit: "100" are rejected too. The evaluator previously accepted them through Integer("100") at check time. YAML-loaded integers are unaffected.

No conformance scenario: the conformance servers are fixtures with valid rules, so construction-time validation is SDK-local.

GitLab impact

GitLab pins gitlab-labkit ~> 5.0.0 and is not affected until it bumps. Two EE placeholder rules there (code_suggestions_x_ray_dependencies, code_suggestions_x_ray_scan) use a static limit: 0, period: 0 and would fail this validation, and since SupportedRateLimits.rules builds every rule in one memoized hash, the first throttled? call after a bump would raise for every key. gitlab-org/gitlab!257222 (merged) switches them to -> { 0 } and adds a Gemfile TODO for the bump; it must merge before GitLab moves to the release carrying this MR. Every other GitLab rule uses a positive literal, an ActiveSupport::Duration (which reports is_a?(Numeric) true), or a callable.

Related Issues

Closes gitlab-com/gl-infra/production-engineering#29762 (closed)

Prerequisite for the GitLab bump: gitlab-org/gitlab!257222 (merged)

🤖 Generated with Claude Code

Edited by Sankalp

Merge request reports