Commit dc3aa483 authored by Sam Wiskow's avatar Sam Wiskow
Browse files

fix(rate_limit): restore log_error in Evaluator rescue block 

Failing open without logging left no signal that something went wrong.
Restores the log_error warn call and passes the logger from Limiter
down to Evaluator so errors are observable.

Co-Authored-By: default avatarClaude Sonnet 4.6 <noreply@anthropic.com>
parent c22497af

lib/labkit/rate_limit/evaluator.rb

+13 −2
Original line number Diff line number Diff line
@@ -12,17 +12,19 @@ module Labkit 
      CHAR_VALUE_MAX_LENGTH = 200

      MISSING_VALUE_SENTINEL = "_unknown_"



      def initialize(name:, rules:, redis:)

      def initialize(name:, rules:, redis:, logger:)

        @name   = name

        @rules  = rules

        @redis  = redis

        @logger = logger

      end



      def check(identifier)

        check_rules(identifier)

      rescue StandardError

      rescue StandardError => e

        # Intentionally broad: fail-open applies to any unexpected error (network,

        # timeout, OOM) not only Redis protocol errors.

        log_error(e, identifier)

        Result.new(matched: false, error: true)

      end
@@ -87,6 +89,15 @@ module Labkit 
        @redis.expire(redis_key, period) if count == 1

        count

      end



      def log_error(error, identifier)

        @logger.warn(

          message: "rate_limit_error",

          name: @name,

          error: error.class.to_s,

          identifier: identifier&.to_h

        )

      end

    end

  end

end

lib/labkit/rate_limit/limiter.rb

+2 −1
Original line number Diff line number Diff line
@@ -26,7 +26,8 @@ module Labkit 
        @evaluator = Evaluator.new(

          name: validated_name,

          rules: rules,

          redis: redis || RateLimit.config.redis

          redis: redis || RateLimit.config.redis,

          logger: resolved_logger

        )

      end

spec/labkit/rate_limit/evaluator_spec.rb

+9 −3
Original line number Diff line number Diff line
@@ -7,6 +7,7 @@ RSpec.describe Labkit::RateLimit::Evaluator do 
  include StubENV



  let(:redis) { instance_double(Redis) }

  let(:null_logger) { instance_double(Labkit::Logging::JsonLogger, warn: nil) }

  let(:identifier) { Labkit::RateLimit::Identifier.new(user: 42, ip: "1.2.3.4") }



  def make_rule(name: "default", match: {}, limit: 100, period: 60, action: :block, characteristics: [:user])
@@ -17,7 +18,7 @@ RSpec.describe Labkit::RateLimit::Evaluator do 
  end



  def evaluator(name: "rack_request", rules: [])

    described_class.new(name: name, rules: rules, redis: redis)

    described_class.new(name: name, rules: rules, redis: redis, logger: null_logger)

  end



  before do
@@ -141,11 +142,16 @@ RSpec.describe Labkit::RateLimit::Evaluator do 
  end



  describe "Scenario S: Redis unavailable" do

    it "returns error Result when Redis is unavailable" do

    it "returns error Result and logs a warning when Redis is unavailable" do

      allow(redis).to receive(:incr).and_raise(RuntimeError, "connection refused")



      logger = instance_double(Labkit::Logging::JsonLogger)

      expect(logger).to receive(:warn).with(

        hash_including(message: "rate_limit_error", error: "RuntimeError")

      )



      rule = make_rule(name: "err_rule")

      result = evaluator(rules: [rule]).check(identifier)

      result = described_class.new(name: "rack_request", rules: [rule], redis: redis, logger: logger).check(identifier)



      expect(result.error?).to be(true)

      expect(result.exceeded?).to be(false)