Commit 0631f853 authored by Max Woolf's avatar Max Woolf
Browse files

feat!: align rate limit actions with the design doc naming

Address reviewer feedback on the rule evaluation model:

- Rename the :block rule action to :limit; rule actions are now
  :limit/:log/:skip and Result#action is only ever :block or :allow.
  An exceeded :log rule now reports action :allow (exceeded? stays true).
- Collect every counted rule evaluation inside Result; the evaluator
  loop returns the accumulator and Result reports the most constraining
  evaluation itself.
- Make Result::Evaluation comparable (Comparable, #<=>) ranking
  blocking first, then exceeded, then fewest remaining, replacing
  more_constraining/constraint_rank.

calls_total keeps the existing action label vocabulary
(allow/block/log/skip), so dashboards are unaffected.

Co-Authored-By: default avatarClaude Fable 5 <noreply@anthropic.com>
parent e79740b9
Loading
Loading
Loading
Loading
Loading
+64 −47
Original line number Diff line number Diff line
@@ -77,8 +77,8 @@ RACK_LIMITER = Labkit::RateLimit::Limiter.new(
- `name` must match `/\A[a-z0-9_]+\z/`. It is used as the first segment of
  every Redis counter key for this limiter, so renaming a `Limiter` abandons
  any in-flight counters.
- `rules` is an ordered array of `Rule` objects. The first rule whose `match`
  hash is satisfied wins (with the exception of `:log` rules — see [Actions](#actions)).
- `rules` is an ordered array of `Rule` objects. Every rule whose `match` hash
  is satisfied is evaluated and counted (see [Evaluation flow](#evaluation-flow)).
- `redis` and `logger` are optional; they fall back to the global
  `Labkit::RateLimit.config` values.

@@ -99,7 +99,7 @@ result = RACK_LIMITER.check(
  endpoint: request.path
)

if result.exceeded? && result.action == :block
if result.action == :block
  response.headers.merge!(result.to_response_headers)
  render plain: "Too Many Requests", status: 429
  return
@@ -115,9 +115,11 @@ the same counter.
`Limiter#peek(identifier)` returns the same `Result` shape but does not
mutate Redis. It is useful when one code path should account for the request
(`check`) and another should gate a side-effect on whether the caller is
already over-limit. `peek` skips `:log` rules — their state is unobservable
without incrementing. A matched `:skip` rule terminates `peek` the same way
it terminates `check`: matched, `:allow`, no Redis read, no `info`.
already over-limit. `peek` reads `:log` rules too: `check` can report a
`:log` rule's evaluation, so excluding them would make `peek` answer a
different question than `check`. A matched `:skip` rule terminates `peek`
the same way it terminates `check`: matched, `:allow`, no Redis read, no
`info`.

## Identifier

@@ -146,7 +148,7 @@ A `Rule` is a `Data.define` value object with the following fields:
| `match`           | Hash of identifier key/value predicates that must **all** be satisfied for the rule to apply. Empty hash matches anything. See [Matchers](#matchers).                |
| `limit`           | Integer request threshold per `period`. May be a callable resolved on every check.                                                                                   |
| `period`          | Window length in seconds. May be a callable resolved on every check.                                                                                                 |
| `action`          | What the result reports when the limit is exceeded. One of `:block`, `:log`, `:skip`. Default `:block`. See [Actions](#actions).                                     |
| `action`          | What the rule does when it matches. One of `:limit`, `:log`, `:skip`. Default `:limit`. See [Actions](#actions).                                                     |
| `characteristics` | Array of identifier keys whose values are folded into the Redis counter key. Each unique combination gets its own counter.                                           |

Making `limit` or `period` callable is the supported pattern for
@@ -185,19 +187,21 @@ each one increments its own counter, and the request debits `cost` from all of
them. Matching alone does not stop the walk. Only two things terminate it:

- a matched `:skip` rule (bypass, nothing counted), and
- a `:block` rule that is **over its limit** — the request is rejected, so the
- a `:limit` rule that is **over its limit** — the request is rejected, so the
  remaining rules cannot change the outcome. Rules declared after it are neither
  counted nor evaluated, meaning a blocked request debits every rule up to and
  including the one that blocked, and none after it.

A `:block` rule that is under its limit does not terminate, so it behaves
A `:limit` rule that is under its limit does not terminate, so it behaves
exactly like `:log` until the moment it blocks.

Because several rules can match without any of them blocking, the single
returned `Result` reports the **most constraining** one: the matched rule with
the fewest requests remaining, ties broken by declaration order. That is what
keeps `to_response_headers` honest — reporting any other matched rule would
advertise headroom that a different rule is about to refuse:
The returned `Result` collects one `Result::Evaluation` per counted rule
(`result.evaluations`), and its readers (`rule`, `info`, `exceeded?`,
`to_response_headers`) report the **most constraining** one: a blocking
evaluation first, then exceeded ones, then the fewest requests remaining,
ties broken by declaration order (the ranking is `Result::Evaluation#<=>`).
That is what keeps `to_response_headers` honest — reporting any other matched
rule would advertise headroom that a different rule is about to refuse:

```
per_ip    limit 1000  count  20  remaining 980
@@ -205,9 +209,9 @@ per_user limit 100 count 99 remaining 1 <- reported
```

An exceeded rule has `remaining` 0 and therefore outranks any rule still under
its limit. For a `:log` rule that means shadow traffic now surfaces to the
caller as `exceeded? == true` with `action == :log` — visible, but still unable
to block. A `:log`-only path that matches therefore returns `matched? == true`
its limit. For a `:log` rule that means shadow traffic surfaces to the caller
as `exceeded? == true` (with `action` still `:allow`) — visible, but unable to
block. A `:log`-only path that matches therefore returns `matched? == true`
and emits no `rule="unmatched"` metric.

```mermaid
@@ -219,14 +223,14 @@ flowchart TD
    Skip -->|"yes (no Redis op)"| SkipEmit[Emit calls_total<br/>action=skip]
    SkipEmit --> SkipReturn([Return matched=true<br/>action=:allow])
    Skip -->|no| Eval["INCR Redis counter<br/>(see Redis sequence below)"]
    Eval --> Build[Build Result<br/>resolve limit/period]
    Build --> Emit[Emit calls_total + limit/period gauges]
    Emit --> Act{over limit<br/>and action=:block?}
    Act -->|yes| Return([Return Result])
    Act -->|"no (:log, or :block under limit)"| Fold[Keep if fewer<br/>remaining than incumbent]
    Fold --> Iter
    Eval --> Build[Build Evaluation<br/>resolve limit/period]
    Build --> Add[Add evaluation to Result]
    Add --> Emit[Emit calls_total + limit/period gauges]
    Emit --> Act{"result.block?<br/>(:limit rule over limit)"}
    Act -->|yes| Return([Return Result<br/>action=:block])
    Act -->|"no (:log, or :limit under limit)"| Iter
    Iter -->|no more rules| Any{any rule<br/>evaluated?}
    Any -->|yes| ReturnFold([Return most-constraining Result])
    Any -->|yes| ReturnFold([Return Result reporting<br/>most-constraining evaluation])
    Any -->|no| Unmatched[Emit calls_total<br/>rule=unmatched, action=allow]
    Unmatched --> ReturnUnmatched([Return matched=false<br/>action=:allow])
    Eval -. StandardError .-> Error[Emit errors_total<br/>log warn]
@@ -243,19 +247,29 @@ Note that `calls_total` is emitted **per matched rule**, so summing it by

### Actions

The rule's `action` controls how the `Result` reports an over-limit hit. The
counter is always incremented when a rule matches, except for `:skip` rules,
which never touch Redis:
The rule's `action` describes what the rule does; the result's `action`
describes the outcome — what the caller should do — and is only ever `:allow`
or `:block`. The counter is always incremented when a rule matches, except for
`:skip` rules, which never touch Redis:

- `:block` — when exceeded, `Result#action` is `:block` and evaluation
| rule action | what it does                                  | exceeded? | result action | terminating?  |
|-------------|-----------------------------------------------|-----------|---------------|---------------|
| `:limit`    | count against the limit                       | no        | `:allow`      | no — continue |
| `:limit`    | count against the limit                       | yes       | `:block`      | yes — stop    |
| `:log`      | count against the limit (observability only)  | no        | `:allow`      | no — continue |
| `:log`      | count against the limit (observability only)  | yes       | `:allow`      | no — continue |
| `:skip`     | don't count (bypass)                          | n/a       | `:allow`      | yes — stop    |

- `:limit` — when exceeded, `Result#action` is `:block` and evaluation
  terminates. Caller should reject the request (e.g. with HTTP 429). When under
  the limit, action is `:allow` and evaluation continues to the next rule.
- `:log`**never terminating**. The rule counts the request and records
  metrics, but evaluation always continues to the next rule. This is the
  mechanism for shadow rules during rollout: stack a `:log` rule and a `:block`
  rule together and the `:log` rule cannot disable the `:block` rule. When
  exceeded, a `:log` rule can still be the rule reported in the `Result`
  (`exceeded? == true`, `action == :log`) if nothing more constraining matched.
  the limit, evaluation continues to the next rule.
- `:log`**never terminating and never blocking**. The rule counts the
  request and records metrics, but evaluation always continues to the next
  rule. This is the mechanism for shadow rules during rollout: stack a `:log`
  rule and a `:limit` rule together and the `:log` rule cannot disable the
  `:limit` rule. When exceeded, a `:log` rule can still be the rule reported in
  the `Result` (`exceeded? == true`, `action == :allow`) if nothing more
  constraining matched.
- `:skip` — bypass. A matching rule terminates evaluation with
  `Result#action` `:allow` **without any Redis operation**: nothing is
  counted, so `limit`, `period`, `characteristics`, and `count_distinct` are
@@ -291,7 +305,7 @@ sequenceDiagram
    alt count == 1 (first write of window)
        E->>R: EXPIRE key period
        R-->>E: 1
        Note over E: ttl returned is -1 here;<br/>build_result falls back to<br/>resolved_period for reset_at.
        Note over E: ttl returned is -1 here;<br/>build_evaluation falls back to<br/>resolved_period for reset_at.
    else count > 1
        Note over E: TTL is not extended:<br/>fixed window from first write.
    end
@@ -304,15 +318,19 @@ issues `EXPIRE`. A missing key (`GET → nil`, `TTL → -2`) is reported as

## Result

`Result` carries the decision back to the caller:
`Result` carries the decision back to the caller. It collects one
`Result::Evaluation` per counted rule; `rule`, `exceeded?`, and `info` report
the most constraining one (see [Evaluation flow](#evaluation-flow)):

```ruby
result.matched?           # => true if some rule matched
result.exceeded?          # => true if the matched rule's counter > limit
result.action             # => :block | :log | :allow
result.rule               # => the matched Rule, or nil
result.exceeded?          # => true if the reported rule's counter > limit
result.action             # => :block | :allow — what the caller should do
result.block?             # => result.action == :block
result.rule               # => the reported Rule, or nil
result.error?             # => true if Redis failed (see Fail-open)
result.info               # => Result::Info or nil
result.evaluations        # => every counted Result::Evaluation, in rule order
result.to_response_headers
# => { "RateLimit-Limit" => "...", "RateLimit-Remaining" => "...", "RateLimit-Reset" => "<unix-ts>" }
```
@@ -334,8 +352,8 @@ safe to merge unconditionally.

The evaluator wraps `check` and `peek` in a broad rescue. Any `StandardError`
(Redis connection failure, timeout, OOM in user-supplied callables, …) is
logged at WARN with `message: "rate_limit_error"` and returned as a
`Result(matched: false, error: true, action: :allow)`. The
logged at WARN with `message: "rate_limit_error"` and returned as an error
`Result` (`matched?` false, `error?` true, `action` `:allow`). The
`gitlab_labkit_rate_limiter_errors_total` counter is incremented. The caller
should treat the request as allowed.

@@ -346,15 +364,14 @@ should treat the request as allowed.

| metric                                          | type    | labels                              | meaning                                                              |
|-------------------------------------------------|---------|-------------------------------------|----------------------------------------------------------------------|
| `gitlab_labkit_rate_limiter_calls_total`        | counter | `rate_limiter`, `rule`, `action`    | One increment per terminating decision; also incremented per matched `:log` rule. `action` is one of `"allow"`, `"block"`, `"log"`, `"skip"`. `rule="unmatched", action="allow"` when no rule terminated. |
| `gitlab_labkit_rate_limiter_calls_total`        | counter | `rate_limiter`, `rule`, `action`    | One increment per counted rule (plus one per matched `:skip` rule). `action` is one of `"allow"` (under limit), `"block"` (blocking `:limit` rule), `"log"` (exceeded `:log` rule), `"skip"`. `rule="unmatched", action="allow"` when no rule matched. |
| `gitlab_labkit_rate_limiter_errors_total`       | counter | `rate_limiter`                      | Fail-open events (any `StandardError` in the labkit path).            |
| `gitlab_labkit_rate_limiter_limit`              | gauge   | `rate_limiter`, `rule`              | Resolved limit at the last check (useful when `limit:` is callable). |
| `gitlab_labkit_rate_limiter_period_seconds`     | gauge   | `rate_limiter`, `rule`              | Resolved period at the last check.                                   |

Because `:log` rules do not terminate, a single `check` call can emit
**multiple** `calls_total` increments: one per `:log` rule that matched, plus
one for the terminating decision (or `rule="unmatched"` if no terminating
rule fired).
Because every matching rule is counted, a single `check` call can emit
**multiple** `calls_total` increments: one per counted rule (or a single
`rule="unmatched"` increment if nothing matched).

## Dev/test vs production guards

+43 −58
Original line number Diff line number Diff line
@@ -72,7 +72,7 @@ module Labkit
        # timeout, OOM) not only Redis protocol errors.
        report_error_metrics
        log_error(e, identifier)
        Result.new(matched: false, error: true, action: :allow)
        Result.error
      end

      # Read-without-increment counterpart to {#check}. Same matching and Result
@@ -83,7 +83,7 @@ module Labkit
      rescue StandardError => e
        report_error_metrics
        log_error(e, identifier)
        Result.new(matched: false, error: true, action: :allow)
        Result.error
      end

      private
@@ -95,14 +95,15 @@ module Labkit
      #   incremented, so the branch sits before the count_distinct check
      #   (identifier completeness is irrelevant to a rule that builds no key).
      #   calls_total still increments so the bypass stays observable.
      # - a :block rule over its limit terminates, because the request is
      # - a :limit rule over its limit terminates, because the request is
      #   rejected and later rules cannot change that. Rules declared after it
      #   are neither counted nor evaluated, so a blocked request debits every
      #   rule up to and including the one that blocked, and none after it.
      #
      # Every other matched rule - :log, and :block while under its limit - is
      # counted and folded into the returned Result by #more_constraining. cost
      # is therefore debited from every matching rule, not just the first.
      # Every other matched rule - :log, and :limit while under its limit - is
      # counted and collected into the returned Result, which reports the
      # most-constraining evaluation (ranking lives in Result::Evaluation#<=>).
      # cost is therefore debited from every matching rule, not just the first.
      #
      # SET-mode rules (rule.count_distinct set) that match but whose identifier
      # is missing the count_distinct key fail open + log + bump errors_total, and
@@ -112,18 +113,18 @@ module Labkit
      # Error handling stays whole-check (see #check): a raise part-way through
      # discards the results of the rules already evaluated, even though their
      # counters were incremented. No blocking verdict is lost that way - a
      # :block rule over its limit returns before any later rule can raise -
      # :limit rule over its limit returns before any later rule can raise -
      # but a rule declared after the failing one loses its chance to block.
      # The request is counted and allowed.
      def check_rules(identifier, cost, rule_context)
        selected = nil
        result = Result.new

        @rules.each do |rule|
          next unless rule_matches?(rule, identifier)

          if rule.action == :skip
            report_skipped_metrics(rule)
            return skip_result(rule)
            return result.skip!(rule)
          end

          if rule.count_distinct && missing_count_distinct_value?(rule, identifier)
@@ -132,66 +133,42 @@ module Labkit
            next
          end

          result = evaluate_rule(rule, identifier, cost, rule_context)
          report_matched_metrics(result)
          return result if result.action == :block

          selected = more_constraining(selected, result)
          evaluation = evaluate_rule(rule, identifier, cost, rule_context)
          result.add_evaluation(evaluation)
          report_matched_metrics(evaluation)
          return result if result.block?
        end

        return selected if selected

        report_unmatched_metrics
        Result.new(matched: false, action: :allow)
        report_unmatched_metrics unless result.matched?
        result
      end

      # Mirror of check_rules without metrics or writes. :log rules are read
      # here too: check can return a :log rule's Result now, so excluding them
      # here too: check can report a :log rule's evaluation, so excluding them
      # would make peek answer a different question than check.
      #
      # peek does not need the count_distinct identifier key - it reads SCARD on
      # the rule-keyed compound key, which contains the cardinality across all
      # members. So missing-key fail-open does not apply here.
      def peek_rules(identifier, rule_context)
        selected = nil
        result = Result.new

        @rules.each do |rule|
          next unless rule_matches?(rule, identifier)

          return skip_result(rule) if rule.action == :skip
          return result.skip!(rule) if rule.action == :skip

          result = peek_rule(rule, identifier, rule_context)
          return result if result.action == :block

          selected = more_constraining(selected, result)
          result.add_evaluation(peek_rule(rule, identifier, rule_context))
          return result if result.block?
        end

        selected || Result.new(matched: false, action: :allow)
      end

      def more_constraining(current, candidate)
        return candidate if current.nil?

        (constraint_rank(candidate) <=> constraint_rank(current)).negative? ? candidate : current
      end

      # remaining is floored at 0, so an exceeded rule ties with one sitting
      # exactly on its limit. Rank exceeded first: a breach must not be hidden
      # by a rule that merely reached its limit and happens to precede it.
      def constraint_rank(result)
        [result.exceeded? ? 0 : 1, result.info.remaining]
        result
      end

      def rule_matches?(rule, identifier)
        rule.match.all? { |key, matcher| matcher.match?(identifier[key]) }
      end

      # A matched :skip rule allows without evaluating: no counter exists, so
      # info is nil (to_response_headers is {} for skip results).
      def skip_result(rule)
        Result.new(matched: true, action: :allow, rule: rule)
      end

      def missing_count_distinct_value?(rule, identifier)
        value = identifier[rule.count_distinct]
        value.nil? || value.to_s.empty?
@@ -211,7 +188,7 @@ module Labkit
            incr_with_ttl(redis_key, resolved_period, cost)
          end

        build_result(rule, resolved_limit, resolved_period, count, ttl)
        build_evaluation(rule, resolved_limit, resolved_period, count, ttl)
      end

      def peek_rule(rule, identifier, rule_context)
@@ -220,12 +197,10 @@ module Labkit
        resolved_period = Integer(resolve_value(rule.period, rule_context))

        count, ttl = rule.count_distinct ? scard_with_ttl(redis_key) : read_with_ttl(redis_key)
        build_result(rule, resolved_limit, resolved_period, count, ttl)
        build_evaluation(rule, resolved_limit, resolved_period, count, ttl)
      end

      def build_result(rule, resolved_limit, resolved_period, count, ttl)
        exceeded = count > resolved_limit
        action = exceeded ? rule.action : :allow
      def build_evaluation(rule, resolved_limit, resolved_period, count, ttl)
        info = Result::Info.new(
          resolved_limit: resolved_limit, resolved_period: resolved_period,
          count: count,
@@ -233,7 +208,7 @@ module Labkit
          reset_at: Time.now.utc + (ttl >= 0 ? ttl : resolved_period)
        )

        Result.new(matched: true, exceeded: exceeded, action: action, rule: rule, info: info)
        Result::Evaluation.new(rule: rule, exceeded: count > resolved_limit, info: info)
      end

      def build_redis_key(rule, identifier)
@@ -362,22 +337,32 @@ module Labkit
        )
      end

      def report_matched_metrics(result)
      def report_matched_metrics(evaluation)
        Metrics.calls_total.increment(
          rate_limiter: @name,
          rule: result.rule.name,
          action: result.action.to_s
          rule: evaluation.rule.name,
          action: calls_action_label(evaluation)
        )
        Metrics.limit_gauge.set(
          { rate_limiter: @name, rule: result.rule.name },
          result.info.resolved_limit
          { rate_limiter: @name, rule: evaluation.rule.name },
          evaluation.info.resolved_limit
        )
        Metrics.period_gauge.set(
          { rate_limiter: @name, rule: result.rule.name },
          result.info.resolved_period
          { rate_limiter: @name, rule: evaluation.rule.name },
          evaluation.info.resolved_period
        )
      end

      # Pre-rename label vocabulary is preserved so dashboards keep working:
      # "block" for a blocking :limit rule, "log" for an exceeded :log rule,
      # "allow" for anything under its limit (the Result-level action for an
      # exceeded :log rule is :allow, but the metric keeps the distinct label).
      def calls_action_label(evaluation)
        return "allow" unless evaluation.exceeded?

        evaluation.block? ? "block" : "log"
      end

      # calls_total carries action="skip" (the rule action, not the :allow the
      # caller sees) so bypass traffic stays distinguishable from counted
      # allows. No limit/period gauges: a skip rule has no limit to report.
+1 −1
Original line number Diff line number Diff line
@@ -15,7 +15,7 @@ module Labkit
    #     rules: [Labkit::RateLimit::Rule.new(name: "api_user", limit: 100, period: 60, characteristics: [:user])]
    #   )
    #   result = limiter.check({ user: 42, ip: "1.2.3.4" })
    #   render_429 if result.exceeded? && result.action == :block
    #   render_429 if result.action == :block
    class Limiter
      NAME_PATTERN = /\A[a-z0-9_]+\z/

+107 −20

File changed.

Preview size limit exceeded, changes collapsed.

+8 −8
Original line number Diff line number Diff line
@@ -2,7 +2,7 @@

module Labkit
  module RateLimit
    KNOWN_ACTIONS = %i[block log skip].freeze
    KNOWN_ACTIONS = %i[limit log skip].freeze
    RULE_NAME_PATTERN = /\A[a-z0-9_]+\z/
    RULE_NAME_MAX_LENGTH = 64

@@ -12,12 +12,12 @@ module Labkit
    #                   the rule to apply; empty hash matches any identifier
    # limit           - request threshold; may be a callable (resolved per check)
    # period          - window in seconds; may be a callable (resolved per check)
    # action          - :block (enforce; terminates evaluation only when over the
    #                   limit), :log (count and log only, never blocks and never
    #                   terminates), or :skip (bypass: permit and terminate
    #                   evaluation on match without counting; performs no Redis
    #                   operation, so limit, period, characteristics, and
    #                   count_distinct are inert)
    # action          - :limit (enforce; the result blocks when the rule is over
    #                   its limit, which also terminates evaluation), :log (count
    #                   and log only, never blocks and never terminates), or
    #                   :skip (bypass: permit and terminate evaluation on match
    #                   without counting; performs no Redis operation, so limit,
    #                   period, characteristics, and count_distinct are inert)
    # characteristics - identifier keys used to build the compound Redis counter key
    # count_distinct  - optional Symbol naming an identifier key. When set, the rule
    #                   counts the number of distinct values seen for that key within
@@ -44,7 +44,7 @@ module Labkit
        sym
      end

      def initialize(name:, limit:, period:, characteristics:, match: {}, action: :block, count_distinct: nil)
      def initialize(name:, limit:, period:, characteristics:, match: {}, action: :limit, count_distinct: nil)
        raise ArgumentError, "name must be a String or Symbol, got #{name.class}" unless name.is_a?(String) || name.is_a?(Symbol)

        name_str = name.to_s
Loading