feat(rate_limit): :log rules continue evaluation; add :allow rule action (Spec 13)

Sam Wiskowrequested to merge
rate-limit/log-rules-no-early-return into master

Summary

Implements Spec 13 / production-engineering#28890. Unblocks Spec 9 / Stage 2b RackAttack migration.

Two structurally identical defects in Evaluator allowed a :log rule to silently disable a subsequent :block rule:

  1. Control-flow defect: check_rules returned on the first matching rule regardless of action — so a :log rule positioned before a :block rule prevented the :block rule from being evaluated. Same defect in peek_rules.
  2. Error-flow defect: Evaluator#check's outer rescue StandardError would unwind the loop on any single rule's failure — so a :log rule's transient Redis error would also stop subsequent :block rules from running.

After this MR:

  • :log rules increment counters and emit metrics but do not early-return. Only :block and :allow rules cause an early return.
  • Rule.new(action: :allow) is now accepted (KNOWN_ACTIONS = %i[block log allow]) and represents a bypass: short-circuits with Result.new(matched: true, action: :allow, rule: rule) and no Redis traffic.
  • Per-rule rescue StandardError isolates rule failures: errors_total increments, the loop continues, and the outer rescue is reserved for things that escape the per-rule path.
  • report_unmatched_metrics fires only when no rule's match: predicate was satisfied (matched_any flag is set on rule_matches?, before evaluate_rule, so partial-failure cases don't spuriously emit the unmatched series).
  • peek_rules mirrors the new semantics: skips past :log rules, short-circuits on :allow, no metrics (peek stays observational).
  • Result action documentation updated to cover the new bypass case.
  • report_matched_metrics guards the limit/period gauges against info.nil? so :allow rule emissions don't NoMethodError.

Behavioral change worth flagging

Existing single-rule callers no longer see Result#error? set on Redis failure (the per-rule rescue catches it before the outer rescue). Telemetry signal moves from Result#error? to the existing gitlab_labkit_rate_limiter_errors_total counter. Verified: no caller in the gitlab monolith or this gem branches on error? for control flow beyond what errors_total already provides; no caller uses action: :allow today.

Test evidence

$ bundle exec rspec --dry-run [the 5 spec files]
153 examples, 0 failures   # BASELINE before this MR

$ bundle exec rspec [the 5 spec files]
173 examples, 0 failures   # 20 new examples added

$ bundle exec rubocop [3 lib + 5 spec files]
8 files inspected, no offenses detected

20 new examples cover Spec 13 acceptance scenarios A–P:

Scenario Where
A: :log alone, exceeds evaluator_spec.rb (Spec 13 block)
B: :log then :block, neither exceeded; asserts info.resolved_limit == 50 same
C: :log then :block, only :block exceeded same
D: :block then :log — early-return preserved same
E: :allow matches — no Redis, headers {}, info nil same
F: :allow before :block (matching) same
G: no rule matches — emits unmatched same
H: two :log rules with distinct names same
I, J: Rule.new(action: :allow) accepted; unknown rejected rule_spec.rb
K: peek skips :log evaluator_spec.rb + integration in rate_limit_spec.rb
L: peek short-circuits :allow same
M: non-matching :allow continues to :block evaluator_spec.rb
N: :allow with match: {} — universal bypass same
O: :log errors, :block still runs same
P: every rule errors → action: :allow, NOT error: true same

Plus integration scenarios in rate_limit_spec.rb for :log shadowing :block over a sequence of requests, and :allow bypass with mixed bypass/non-bypass identifiers.

Updated existing tests

The single-rule :log-rule and Redis-error cases changed behavior. Three matrix rows in rate_limit_spec.rb and three single-rule failure tests in evaluator_spec.rb were updated to reflect the new contract. No examples were removed; only their assertions were updated to match the new semantics. The instance_double(Logger) and instance_double(Labkit::Logging::JsonLogger) setups were extended to stub :error (used by the new log_rule_error helper).

Spec process

  • Two adversarial review rounds (4 + 2 BLOCKERs surfaced and resolved before code was written): see round 1, round 2, resolutions.

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

🤖 Generated with Claude Code

Merge request reports