Commit e79740b9 authored by Max Woolf's avatar Max Woolf
Browse files

feat!: evaluate every matching rate limit rule

Previously `check` was first-match-wins: the first rule whose match
conditions were satisfied terminated evaluation, so a later, tighter rule
never counted and never blocked. Only `:log` rules were non-terminating.

Now every matching rule is evaluated and counted. Two cases still terminate:
a matched `:skip` rule, and a `:block` rule that is over its limit (there is
nothing left to learn once the request is rejected). A `:block` rule under
its limit behaves exactly like `:log`.

Because several rules can match without any of them blocking, the single
returned Result reports the most constraining one: fewest `remaining`, ties
broken by declaration order. `remaining` is floored at 0, so an exceeded rule
would tie with one sitting exactly on its limit; exceeded is ranked first so a
breach is never hidden by a rule that merely reached its limit.

`peek` mirrors this and no longer excludes `:log` rules, since `check` can now
report a `:log` rule's Result.

Caller-visible changes:

- `cost` is debited from every matching rule, not just the first.
- A `:log`-only path returns `matched? == true` and emits no `rule="unmatched"`
  metric; an exceeded `:log` rule surfaces as `exceeded? == true` with
  `action == :log`. Shadow rules are now observable without gaining the
  ability to block.
- `calls_total` is emitted per matched rule, so summing it by `rate_limiter`
  counts rule evaluations rather than requests.
- `RateLimit-Reset` may point further out, since `remaining` is compared
  across rules with different periods.

Error handling stays whole-check: a raise part-way through discards the
verdicts of rules already evaluated, though their counters were incremented.
No blocking verdict is lost that way, but a rule declared after the failing
one loses its chance to block.

Co-Authored-By: default avatarClaude Opus 5 (1M context) <noreply@anthropic.com>
parent fa04d0a3
Loading
Loading
Loading
Loading
Loading
+2 −1
Original line number Diff line number Diff line
@@ -40,7 +40,8 @@ module Labkit
      #
      # @param name [String] call site name
      # @param identifier [Identifier, Hash] caller attributes
      # @param rules [Array<Rule>] ordered list of rules (first match wins)
      # @param rules [Array<Rule>] ordered list of rules; every matching rule is
      #   counted, and the Result reports the most constraining one
      # @param redis [Object, nil] Redis client; falls back to config.redis
      # @param logger [Logger, nil] logger; falls back to config.logger
      # @param cost [Numeric] amount to add to the counter; see Limiter#check
+53 −16
Original line number Diff line number Diff line
@@ -180,10 +180,35 @@ Glob and prefix matchers are intentionally out of scope.

### Evaluation flow

`check` walks the rule list in order. The first **terminating** rule wins;
`:log` rules count but do not terminate, so they cannot disable a following
`:block` rule. A pure `:log`-only path still emits one `rule="unmatched"`
metric increment because no terminating rule fired.
`check` walks the rule list in order and evaluates **every** rule that matches:
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
  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
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:

```
per_ip    limit 1000  count  20  remaining 980
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`
and emits no `rule="unmatched"` metric.

```mermaid
flowchart TD
@@ -196,29 +221,41 @@ flowchart TD
    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{rule.action}
    Act -->|":log<br/>(non-terminating)"| Iter
    Act -->|:block| Return([Return Result])
    Iter -->|no more rules| Unmatched[Emit calls_total<br/>rule=unmatched, action=allow]
    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
    Iter -->|no more rules| Any{any rule<br/>evaluated?}
    Any -->|yes| ReturnFold([Return most-constraining Result])
    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]
    Error --> ReturnErr([Return error=true<br/>action=:allow])
```

Error handling is whole-check, not per-rule: a Redis failure part-way through
discards the verdicts of the rules already evaluated, even though their counters
were incremented. Those requests are counted but produce no verdict — the
fail-open trade-off is that a request is never blocked on a partial evaluation.

Note that `calls_total` is emitted **per matched rule**, so summing it by
`rate_limiter` counts rule evaluations, not requests.

### 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:

- `:block` — when exceeded, `Result#action` is `:block`. Caller should reject
  the request (e.g. with HTTP 429). When under the limit, action is `:allow`.
- `:log`**non-terminating**. The rule counts the request and records
  metrics, but evaluation 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. Note that a
  pure `:log`-only check still emits one `rule="unmatched"` metric entry
  because no terminating rule fired.
- `:block` — 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.
- `: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
+48 −9
Original line number Diff line number Diff line
@@ -88,19 +88,36 @@ module Labkit

      private

      # :log rules are non-terminating: they emit metrics and continue,
      # so a shadow :log rule cannot disable a following :block rule.
      # Every rule that matches is evaluated and counted; matching does not
      # short-circuit the loop. Exactly two cases terminate it early:
      #
      # :skip rules terminate on match without touching Redis: no counter is
      # - :skip terminates on match without touching Redis. No counter is
      #   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
      #   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.
      #
      # 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
      # the loop continues to the next rule (the rule is treated as not applicable
      # rather than aborting the whole evaluation).
      #
      # 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 -
      # 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

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

@@ -117,30 +134,52 @@ module Labkit

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

          selected = more_constraining(selected, result)
        end

        return selected if selected

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

      # Mirror of check_rules without metrics: peek skips :log rules (their state
      # is unobservable through peek).
      # 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
      # 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

        @rules.each do |rule|
          next if rule.action == :log
          next unless rule_matches?(rule, identifier)

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

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

          selected = more_constraining(selected, result)
        end

        Result.new(matched: false, action: :allow)
        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]
      end

      def rule_matches?(rule, identifier)
+4 −3
Original line number Diff line number Diff line
@@ -5,9 +5,10 @@ module Labkit
    module Metrics
      module_function

      # :log rules are non-terminating: a check that matched only :log rules
      # increments calls_total once per matched :log rule AND once with
      # rule="unmatched", action="allow", since no terminating decision was made.
      # Emitted once per *matched rule*, not once per check: every rule that
      # matches is evaluated, so summing this by rate_limiter counts rule
      # evaluations rather than requests. rule="unmatched", action="allow" is
      # emitted only when no rule matched at all.
      def calls_total
        Labkit::Metrics::Client.counter(
          :gitlab_labkit_rate_limiter_calls_total,
+15 −5
Original line number Diff line number Diff line
@@ -3,8 +3,15 @@
module Labkit
  module RateLimit
    # Result is the return value of Limiter#check.
    # matched?  - true if a rule's match conditions were satisfied
    # exceeded? - true if the matched rule's counter exceeded its limit
    #
    # Every rule whose match conditions are satisfied is evaluated and counted;
    # the Result reports the single most-constraining one - the matched rule
    # with the fewest requests remaining, ties broken by declaration order. A
    # :block rule that is over its limit short-circuits evaluation and is always
    # the rule reported, as does a matched :skip rule.
    #
    # matched?  - true if at least one rule's match conditions were satisfied
    # exceeded? - true if the reported rule's counter exceeded its limit
    # action    - the outcome: what the caller should do
    #             :block = rule matched, exceeded, rule configured to block
    #             :log   = rule matched, exceeded, rule configured to log only
@@ -12,10 +19,13 @@ module Labkit
    #                      to skip (bypass, nothing counted), no rule matched,
    #                      or error (fail-open)
    #             The rule's configured action is available via rule.action.
    # rule      - the matched Rule object (nil when matched? is false)
    # rule      - the most-constraining matched Rule (nil when matched? is false).
    #             Other rules may also have matched and been counted; only this
    #             one is reported.
    # error?    - true if Redis was unavailable; result fails open (exceeded? is false)
    # info      - Result::Info with per-window counters; nil when matched? is false,
    #             error?, or the matched rule is :skip (no counter exists)
    # info      - Result::Info with per-window counters for the reported rule;
    #             nil when matched? is false, error?, or the matched rule is
    #             :skip (no counter exists)
    Result = Data.define(:matched, :exceeded, :action, :rule, :error, :info) do
      def initialize(matched:, action: nil, exceeded: false, rule: nil, error: false, info: nil)
        super
Loading