Parent epic: [gitlab-com/gl-infra#2021](https://gitlab.com/groups/gitlab-com/gl-infra/-/work_items/2021) (Phase 2: Rate Limiting Simplification) Context: https://gitlab.com/gitlab-org/ruby/gems/labkit-ruby/-/commit/26d1d9a240917e4a6b882bb7c64c9df21de2b261#note_3321679402 Spec revision: round 3 (resolves [adversarial review round 1 note_3323510310](https://gitlab.com/gitlab-com/gl-infra/production-engineering/-/work_items/28890#note_3323510310) and [round 2 note_3323577073](https://gitlab.com/gitlab-com/gl-infra/production-engineering/-/work_items/28890#note_3323577073) — 4+2 BLOCKERs and 10+4 CONCERNs across two adversarial passes). # [Spec 13] Fix: `:log` rules must not early-return in rule evaluation ## Spec ### Problem Statement [required] `Labkit::RateLimit::Evaluator#check_rules` returns immediately on the first matching rule, regardless of `rule.action`: ```ruby # lib/labkit/rate_limit/evaluator.rb (current) def check_rules(identifier) @rules.each do |rule| next unless rule_matches?(rule, identifier) result = evaluate_rule(rule, identifier) report_matched_metrics(result) return result # <-- early return for ALL actions, including :log end ... end ``` Given a Limiter configured with: ```ruby rules: [ { name: "test_new_limit", action: :log, limit: 5, ... }, { name: "authenticated_api", action: :block, limit: 20, ... } ] ``` the `:log` rule matches first, increments its counter, and returns. The `:block` rule is never evaluated and the request is never counted against the live enforcement counter. This silently disables every `:block` rule that follows a `:log` rule in the rule list — which defeats the entire purpose of `:log` mode (shadow-test a new rule alongside live enforcement, without disrupting it). A second, structurally identical defect lurks in error handling: `Evaluator#check` (`lib/labkit/rate_limit/evaluator.rb:22–30`) wraps `check_rules` in a top-level `rescue StandardError` that fails open. After fixing the early-return, a transient Redis error inside a `:log` rule's `evaluate_rule` would unwind the loop via the outer `rescue`, the subsequent `:block` rule would never be evaluated, and the bug would re-appear under transient errors. This spec eliminates both shapes of the defect. The same early-return defect exists in `Evaluator#peek_rules`, which mirrors the `check_rules` flow for read-only inspection. This is a hard blocker for [Spec 9 / Stage 2b — RackAttack migration](https://gitlab.com/gitlab-com/gl-infra/production-engineering/-/work_items/28852), which requires: - `:log` rules to shadow-test new throttles alongside live `:block` rules in the same `Limiter`. - A new `:allow` rule action to express RackAttack `safelist` / bypass behaviour (`bypass_header`, `user_allowlist`, etc.) inside the same `Limiter`. Today, `Rule.new(action: :allow)` raises `ArgumentError` (`KNOWN_ACTIONS = [:block, :log]` at `lib/labkit/rate_limit/rule.rb:5`). ### Non-Goals [required] - Implementing the actual RackAttack migration ([Spec 9 / #28852](https://gitlab.com/gitlab-com/gl-infra/production-engineering/-/work_items/28852)). This spec only unblocks it. - Relaxing `Rule#initialize` so `limit:` / `period:` become optional for `:allow` rules. Out of scope; bypass rules still pass any value (commonly `1` / `1`) until that follow-up lands. - Regex-timeout fail-open handling ([#28882](https://gitlab.com/gitlab-com/gl-infra/production-engineering/-/work_items/28882)). - Changes to `Rule#match` semantics, rule-name validation, or the metrics schema. - Changes to `Result` field shape — only the action-source documentation comment is updated. - Migrating existing `Limiter` callers — no caller uses `action: :allow` today (verified across labkit-ruby and the gitlab monolith). - Dashboard changes to Spec 12 ([#28831](https://gitlab.com/gitlab-com/gl-infra/production-engineering/-/work_items/28831)). The new `(rule, action: allow)` series this fix produces is acknowledged as a rollout smoke-check, not a dashboard PR. ### Error-handling design (referenced by acceptance criteria) Each `evaluate_rule` invocation in `check_rules` is wrapped in a per-rule `rescue StandardError`. On failure for a single rule: - `Metrics.errors_total{rate_limiter: @name}` is incremented. - `@logger.error(message: "rate_limit_rule_evaluation_error", rate_limiter: @name, rule: rule.name, error_class: e.class.name, error_message: e.message)` is emitted. (Includes `rate_limiter: @name` so 3am triage can identify which Limiter failed without grepping for rule names across every Limiter that defines it.) - `calls_total` is **not** incremented for the failed rule. - The loop continues to the next rule. **`matched_any` flag (gates `report_unmatched_metrics`):** set as soon as a rule's `match:` predicate is satisfied (i.e. immediately after `rule_matches?` returns true), **before** `evaluate_rule` is called. This ensures that a transient error during a `:log` rule's evaluation does not cause the loop to spuriously emit `report_unmatched_metrics` after the loop completes — the rule did match, it just failed to evaluate. `report_unmatched_metrics` fires only when no rule's `match:` predicate was satisfied. **Rescue scope:** per-rule rescue catches `StandardError`, matching the outer rescue's scope at `evaluator.rb:24`. `Exception` subclasses (`SystemExit`, `Interrupt`, `NoMemoryError`, `SignalException`) intentionally bypass both rescues — process-fatal conditions are not the evaluator's concern. The outer `rescue StandardError` in `Evaluator#check` is retained as a safety net for anything that escapes the per-rule rescue (e.g. a defect in `report_matched_metrics` itself, or in the `matched_any` gate). When it fires, `Result.new(matched: false, error: true, action: :allow)` is returned as today. This isolates rule errors so a `:log` rule's transient Redis blip cannot disable subsequent `:block` rules. ### Acceptance Criteria [required] In every scenario, "Redis pool is not invoked" means `@redis.with` is not called. "Raw Redis does not receive `:pipelined` / `:expire`" is the equivalent assertion against the existing `instance_double(Redis)` test harness at `spec/labkit/rate_limit/evaluator_spec.rb:9–13`. Implementers should pin whichever assertion matches the harness they use; the load-bearing claim is "no Redis traffic for this rule". **Multi-rule scenarios always use distinct rule names.** `prepare_rules` (`lib/labkit/rate_limit/limiter.rb:77–96`) raises `ArgumentError` on duplicate names in dev/test. Use `log_rule_a` / `block_rule_a` / `allow_rule_a` style names rather than `make_rule(...)` helper defaults, which collide on `name: "default"`. **Scenarios O and P stub `evaluate_rule` to raise a `StandardError` subclass** (e.g. `Redis::CannotConnectError`, `RuntimeError`). `Exception`-tier classes (`SystemExit`, `Interrupt`) are not within the per-rule rescue's scope and are out of scope for these tests. **Scenario A: `:log` rule alone, matches and exceeds** - Given: a Limiter with one rule `{action: :log, limit: 1}` and an identifier whose counter is already above the limit. - When: `Limiter#check(identifier)` is called. - Then: - Redis counter for the `:log` rule is incremented. - `calls_total{rule: <log_rule>, action: log}` is incremented. - `calls_total{rule: unmatched, action: allow}` is **NOT** incremented. - Returned `Result == Result.new(matched: false, action: :allow)`. **Scenario B: `:log` rule then `:block` rule, both match, neither exceeded** - Given: a Limiter with `[log_rule(limit: 1000), block_rule(limit: 50)]`, both matching the same identifier; both counters below their limits. - When: `Limiter#check(identifier)` is called. - Then: - Redis counters for **both** rules are incremented. - `calls_total{rule: <log_rule>, action: allow}` and `calls_total{rule: <block_rule>, action: allow}` are both incremented (because `Evaluator#build_result` resolves `action: :allow` when not exceeded — see `lib/labkit/rate_limit/evaluator.rb:92`). - Returned `Result.action == :allow`, `Result.matched == true`, `Result.rule == block_rule` (the last evaluated rule wins for the unexceeded case). - `Result.info.resolved_limit == 50` (the `:block` rule's limit, not the `:log` rule's). This pins the "last evaluated rule wins" decision so `to_response_headers` advertises the enforcement limit, not the shadow limit. **Scenario C: `:log` rule then `:block` rule, only `:block` exceeded** - Given: same Limiter as Scenario B, but the `:block` rule's counter is already past its limit while the `:log` rule's is below. - When: `Limiter#check(identifier)` is called. - Then: - Both Redis counters are incremented. - `calls_total{rule: <log_rule>, action: allow}` and `calls_total{rule: <block_rule>, action: block}` are both incremented. - Returned `Result.action == :block`, `Result.rule == block_rule`. **Scenario D: `:block` rule then `:log` rule (existing behaviour preserved)** - Given: a Limiter with `[block_rule(limit: 1), log_rule(limit: 1)]`, both matching, the `:block` rule already exceeded. - When: `Limiter#check(identifier)` is called. - Then: - Redis counter for the `:block` rule is incremented; counter for the `:log` rule is **NOT**. - Returned `Result.action == :block`. - Confirms early-return on `:block` is unchanged. (This is the same shape as the existing first-match-wins test at `spec/labkit/rate_limit_spec.rb:94–104`.) **Scenario E: `:allow` rule matches** - Given: a Limiter with one rule `{action: :allow, match: {bypass: true}, limit: 1, period: 60}` and an identifier where `bypass: true`. - When: `Limiter#check(identifier)` is called. - Then: - Redis pool is not invoked: `raw_redis` does not receive `:pipelined` and does not receive `:expire` (or `@redis.with` is never called, depending on harness). - `calls_total{rule: <allow_rule>, action: allow}` is incremented. - Returned `Result == Result.new(matched: true, action: :allow, rule: allow_rule)`. - `result.exceeded == false`, `result.error == false`, `result.info == nil`. - `result.to_response_headers == {}` (bypass returns no rate-limit headers; `Result#to_response_headers` returns `{}` when `info.nil?` per `lib/labkit/rate_limit/result.rb:37–45`). **Scenario F: `:allow` rule before `:block` rule (matching identifier)** - Given: a Limiter with `[allow_rule(match: {bypass: true}), block_rule(limit: 1)]`. Identifier has `bypass: true`. - When: `Limiter#check(identifier)` is called. - Then: - The `:allow` rule short-circuits. - Redis counter for the `:block` rule is **NOT** incremented; the `:block` rule is not evaluated at all. - Returned `Result.action == :allow`, `Result.rule == allow_rule`. **Scenario G: no rule matches (genuine fall-through)** - Given: a Limiter whose rules' `match:` conditions are not satisfied by the identifier. - When: `Limiter#check(identifier)` is called. - Then: - No Redis call is made. - `calls_total{rule: unmatched, action: allow}` **IS** incremented. - Returned `Result == Result.new(matched: false, action: :allow)`. **Scenario H: two `:log` rules, both match — distinct names required** - Given: a Limiter with two `:log` rules **with distinct names** (`log_rule_a`, `log_rule_b`), both matching the same identifier. (`prepare_rules` at `lib/labkit/rate_limit/limiter.rb:77–96` deduplicates by sanitized name and raises `ArgumentError` in dev/test on duplicates, so the test must use distinct names.) - When: `Limiter#check(identifier)` is called. - Then: - Redis counters for both rules are incremented. - `calls_total` is incremented once per rule (`{rule: log_rule_a, action: ...}` and `{rule: log_rule_b, action: ...}`). - `calls_total{rule: unmatched, action: allow}` is **NOT** incremented. - Returned `Result == Result.new(matched: false, action: :allow)`. **Scenario I: `Rule.new(action: :allow)` is accepted** - Given: `Rule.new(name: "bypass", action: :allow, limit: 1, period: 60, characteristics: [:user])`. - When: the Rule object is constructed. - Then: - No `ArgumentError` is raised. - `rule.action == :allow`. - String `"allow"` is also coerced to `:allow`. **Scenario J: `Rule.new(action: <unknown>)` still rejected** - Given: `Rule.new(name: "x", action: :deny, ...)`. - When: the Rule object is constructed. - Then: `ArgumentError` is raised with a message listing `[:block, :log, :allow]`. Confirms the validation surface stays tight. **Scenario K: `Limiter#peek` skips `:log` rules** - Given: a Limiter with `[log_rule, block_rule]`, both matching. - When: `Limiter#peek(identifier)` is called. - Then: - Returned `Result` reflects the `:block` rule's read-only state. - No Redis writes occur (peek path). - No metrics are emitted (peek is observational; current behaviour preserved per `evaluator_spec.rb:521–526`). **Scenario L: `Limiter#peek` short-circuits on `:allow` rule** - Given: a Limiter with `[allow_rule(match: {bypass: true}), block_rule]`. Identifier has `bypass: true`. - When: `Limiter#peek(identifier)` is called. - Then: - Returned `Result == Result.new(matched: true, action: :allow, rule: allow_rule)`. - No Redis call is made for either rule. No metrics are emitted (consistent with Scenario K and `evaluator_spec.rb:521–526`). **Scenario M: `:allow` rule does NOT match — evaluation continues** - Given: a Limiter with `[allow_rule(match: {bypass: true}, action: :allow), block_rule(limit: 1)]`. Identifier is `{user: 1}` (no `bypass` key). - When: `Limiter#check(identifier)` is called twice (counter exceeds on second call). - Then: - The `:allow` rule does **not** short-circuit (its `match:` is not satisfied; `rule_matches?` returns false). - The `:allow` rule contributes no Redis writes and no metrics on these calls. - Redis counter for the `:block` rule is incremented on every call. - Second call returns `Result.action == :block`, `Result.rule == block_rule`. **Scenario N: `:allow` rule with `match: {}` — universal bypass** - Given: a Limiter with `[allow_rule(match: {}, action: :allow, limit: 1, period: 60), block_rule(limit: 1)]`. Identifier is arbitrary (`{user: 42}`). - When: `Limiter#check(identifier)` is called repeatedly (e.g. 10 times). - Then: - The `:allow` rule short-circuits for every identifier (empty `match:` matches vacuously via `Hash#all?`). - Redis pool is not invoked (`@redis.with` not called) for the `:allow` rule. - Returned `Result == Result.new(matched: true, action: :allow, rule: allow_rule)` on every call. - The `:block` rule is never evaluated; its Redis counter remains zero across all calls. (This pins the "structural risk" called out in the Security section: a global bypass behaves exactly as configured. Misconfiguration is a configuration concern, not an evaluator one.) **Scenario O: `:log` rule errors mid-loop, `:block` rule still runs** - Given: a Limiter with `[log_rule, block_rule(limit: 1)]`, both matching. The `:log` rule's `evaluate_rule` is stubbed to raise `Redis::CannotConnectError`. The `:block` rule's Redis path is healthy and its counter is already past its limit. - When: `Limiter#check(identifier)` is called. - Then: - The per-rule `rescue StandardError` catches the `:log` rule's failure. - `Metrics.errors_total{rate_limiter: <name>}` is incremented exactly once (for the `:log` rule's failure). - `Metrics.calls_total{rule: <log_rule>, ...}` is **NOT** incremented (the rule errored before metrics were emitted). - The loop continues to the `:block` rule. Its Redis counter is incremented; `calls_total{rule: <block_rule>, action: block}` is incremented. - Returned `Result.action == :block`, `Result.rule == block_rule`. The outer `rescue` is **not** triggered. - `@logger` receives an `error`-level call with `message: "rate_limit_rule_evaluation_error", rule: <log_rule.name>, error_class: "Redis::CannotConnectError"`. **Scenario P: every rule errors — fall-through is allow, not error** - Given: a Limiter with `[log_rule, block_rule]`, both matching. Both rules' `evaluate_rule` raise a `StandardError` subclass (e.g. `Redis::CannotConnectError`). - When: `Limiter#check(identifier)` is called. - Then: - `Metrics.errors_total` is incremented twice. - No `calls_total` increments occur. - `matched_any` is set to `true` (both rules' `match:` predicates were satisfied — the gate is on the `match:` predicate, not on successful evaluation; see Error-handling design above). - `report_unmatched_metrics` does **not** fire (because `matched_any == true`). - Returned `Result == Result.new(matched: false, action: :allow)` (NOT `error: true`). - The outer `rescue` is **not** triggered (per-rule rescues caught both errors). ### Security Considerations [required] - **Bypass-rule abuse (catch-all `match: {}`):** introducing `:allow` as a rule action introduces an explicit bypass path. A misconfigured bypass rule with `match: {}` globally disables rate limiting on a Limiter. **Scenario N pins this as the configured behaviour** — the evaluator honours the rule list it is given. Operational mitigation: every `:allow` match increments `calls_total{action: allow}`, so bypass usage is observable on the Rate Limiting Overview dashboard ([Spec 12 / #28831](https://gitlab.com/gitlab-com/gl-infra/production-engineering/-/work_items/28831)). Configuration validation (e.g. forbidding `match: {}` on `:allow` rules) is a separate concern and is intentionally out of scope here. - **`:log`-mode silent enforcement gap (the existing bug):** today, adding any `:log` rule before a `:block` rule silently disables the `:block` rule (via early-return). A transient Redis error in a `:log` rule does the same (via outer `rescue`). Operators believe enforcement is in place when it is not. This spec eliminates both shapes — Scenarios B/C cover control-flow, Scenarios O/P cover error-flow. - **Config injection / value exposure:** none new. Rule values (`limit`, `period`, `action`) are not derived from user-controlled input at the evaluator boundary. - **Privilege escalation:** none new. Rule actions are accepted from a fixed enum (`:block`, `:log`, `:allow`); arbitrary values still raise `ArgumentError` at `Rule.new` (Scenario J). ### Rollout & Backwards Compatibility [required] - **Self-managed:** ships as a labkit-ruby gem release; consumers pick it up at next bundle update. No YAML or config schema changes. (Verified: nothing in `lib/labkit/rate_limit/` reads from disk or env at load time.) - **Dedicated:** same as Self-managed. - **Cells:** same as Self-managed (no per-cell config involved). - **No feature flag.** This is a labkit-ruby-internal evaluation-loop fix. Existing single-rule callers (Cohort 1 in production at 100%, Cohort 2 in flag rollout) are unaffected because: - `:block` rules still early-return (Scenario D). - There are no `:log` rules in production today. - `:allow` is a new value that no caller passes yet. - **Backwards compatibility:** - `Rule.new(action: :block)` and `Rule.new(action: :log)` — unchanged. - `Rule.new(action: :allow)` — newly accepted (was `ArgumentError`). - `Result` shape — unchanged. Only the action-source documentation comment is updated. - **Error semantics change:** previously, any single rule's `evaluate_rule` failure caused `Evaluator#check` to return `Result.new(matched: false, error: true, action: :allow)`. After the fix, single-rule failures are isolated and the loop continues; only when ALL rules have errored or no rule matches does the caller see a fall-through `Result.new(matched: false, action: :allow)`. This is a strict superset of fail-open (still allows on error) but the `error: true` flag is no longer set in the multi-rule partial-failure case. Existing single-rule callers see no behavioural change because there is no second rule to continue to. - Existing test "Scenario E: first-match-wins" (`spec/labkit/rate_limit_spec.rb:94–104`) tests two `:block` rules; behaviour preserved by Scenario D above. - **Rollout smoke-check:** after the gem is bumped in gitlab-rails, glance at the Rate Limiting Overview dashboard ([Spec 12 / #28831](https://gitlab.com/gitlab-com/gl-infra/production-engineering/-/work_items/28831)) for unexpected new `(rule, action)` series. Expected: no new series until a Limiter actually starts using `:log` or `:allow` rules in production. If new series appear without a corresponding rule-config change, that's a regression signal. - **Sequencing:** must ship before [Spec 9 / Stage 2b RackAttack migration](https://gitlab.com/gitlab-com/gl-infra/production-engineering/-/work_items/28852) opens its first MR. ### Validation Loop / Verification Process [required] Run from the labkit-ruby repo root inside the worktree (`wt switch --create rate-limit/log-rules-no-early-return`): **Step 1 — pin baseline example count BEFORE adding new tests:** ```bash bundle exec rspec --dry-run \ spec/labkit/rate_limit_spec.rb \ spec/labkit/rate_limit/evaluator_spec.rb \ spec/labkit/rate_limit/rule_spec.rb \ spec/labkit/rate_limit/result_spec.rb \ spec/labkit/rate_limit/limiter_spec.rb \ | grep -E '^[0-9]+ examples?,' ``` Record the example count (call it `BASELINE`). The MR description must include this number. Note: `tail -1` does NOT work — rspec's default formatter emits a trailing blank line that `tail -1` would capture instead of the count. **Step 2 — full suite must pass after implementation:** ```bash bundle exec rspec \ spec/labkit/rate_limit_spec.rb \ spec/labkit/rate_limit/evaluator_spec.rb \ spec/labkit/rate_limit/rule_spec.rb \ spec/labkit/rate_limit/result_spec.rb \ spec/labkit/rate_limit/limiter_spec.rb \ --format documentation ``` Expectation: `BASELINE + N_new` examples, 0 failures, where `N_new` is the count of new Scenario A–P examples added in this MR. Removal of any pre-existing example is a regression and must be justified in the MR. **Step 3 — lint:** ```bash bundle exec rubocop \ lib/labkit/rate_limit/evaluator.rb \ lib/labkit/rate_limit/rule.rb \ lib/labkit/rate_limit/result.rb \ spec/labkit/rate_limit/evaluator_spec.rb \ spec/labkit/rate_limit/rule_spec.rb \ spec/labkit/rate_limit/result_spec.rb \ spec/labkit/rate_limit/limiter_spec.rb \ spec/labkit/rate_limit_spec.rb ``` Expectation: no offenses. (rspec and rubocop file lists are now aligned.) **Step 4 — evidence on the MR:** CI pipeline URL, the `bundle exec rspec ... --format documentation` output (or its trailing summary), and the `BASELINE → BASELINE + N_new` count delta posted as a comment on the MR before requesting human review. ### Observability [optional] The fix is observable through existing labkit Prometheus counters — no new metrics are introduced. - `gitlab_labkit_rate_limiter_calls_total{rate_limiter, rule, action}` (already emitted by `Evaluator#report_matched_metrics`; metric name confirmed at `lib/labkit/rate_limit/metrics.rb:9`). After the fix, multi-rule scenarios produce one increment per matched rule (instead of one increment for the first-matched rule only): - One series per `:log` rule plus one per `:block` / `:allow` rule that matches. - `rule: "unmatched"` only fires when no rule's `match:` predicate was satisfied (Scenario G is the only path that emits it). - **New series possible:** `gitlab_labkit_rate_limiter_calls_total{rule: <log_rule_name>, action: allow}` did not exist before this fix (the early-return prevented `:log` rules from emitting `action: allow` for non-exceeded counts). Operators should expect to see these once a `:log` rule lands in production. This is the rollout smoke-check signal in the previous section. - `gitlab_labkit_rate_limiter_errors_total{rate_limiter}` (already emitted on Redis errors; confirmed at `lib/labkit/rate_limit/metrics.rb:18`). After the fix, individual rule errors increment this without aborting the check — Scenarios O and P cover this. - The "Rate Limiting: Overview" dashboard ([Spec 12 / #28831](https://gitlab.com/gitlab-com/gl-infra/production-engineering/-/work_items/28831)) inherits this for free; no dashboard changes required as part of this spec. - **3am triage diagnostic:** if a shadow `:log` rule is silently disabling enforcement (i.e. the regression is back), both `:log` and `:block` rule labels should appear with non-zero rate. Run: ```promql sum by (rule, action) (rate(gitlab_labkit_rate_limiter_calls_total{rate_limiter="<limiter_name>"}[5m])) ``` Expected: rows for both the `:log` rule and the `:block` rule with non-zero rate. If the `:log` rule is non-zero but the `:block` rule is missing entirely (or vice versa for an `:allow` rule), the fix has regressed.