From c20d612dbce87c8bd53453d24977ef0534263d6d Mon Sep 17 00:00:00 2001 From: Gil Desmarais Date: Fri, 12 Jun 2026 12:56:01 +0200 Subject: [PATCH 1/4] Address review feedback for PR #1011 --- app/web/errors/error_responder.rb | 11 +++++----- app/web/request/rate_limiter.rb | 10 +++++++-- spec/html2rss/web/error_responder_spec.rb | 26 +++++++++++++++++++++++ 3 files changed, 39 insertions(+), 8 deletions(-) diff --git a/app/web/errors/error_responder.rb b/app/web/errors/error_responder.rb index 78dc6f93..7695cf8f 100644 --- a/app/web/errors/error_responder.rb +++ b/app/web/errors/error_responder.rb @@ -25,7 +25,7 @@ class << self # rubocop:disable Metrics/ClassLength # @param response [Rack::Response] # @param error [StandardError] # @return [String] - # rubocop:disable Metrics/AbcSize, Metrics/MethodLength + # rubocop:disable Metrics/AbcSize, Metrics/MethodLength, Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity def respond(request:, response:, error:) status = resolve_status(error) code = resolve_error_code(error) @@ -34,7 +34,7 @@ def respond(request:, response:, error:) if status == 429 response['Retry-After'] ||= Flags.rate_limit_window_seconds.to_s elsif [503, 504].include?(status) - response['Retry-After'] = Flags.retry_after_timeout_seconds.to_s + response['Retry-After'] ||= Flags.retry_after_timeout_seconds.to_s end emit_error_event(error, code, response.status) @@ -48,7 +48,7 @@ def respond(request:, response:, error:) render_xml_error(response, error) end - # rubocop:enable Metrics/AbcSize, Metrics/MethodLength + # rubocop:enable Metrics/AbcSize, Metrics/MethodLength, Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity private @@ -74,9 +74,8 @@ def render_xml_error(response, error) def resolve_error_code(error) case error when ->(e) { extraction_empty_failure?(e) } then EXTRACTION_EMPTY_CODE - when TooManyRequestsError then 'TOO_MANY_REQUESTS' - when ServiceUnavailableError, ->(e) { server_timeout?(e) } then 'SERVICE_UNAVAILABLE' - when GatewayTimeoutError, ->(e) { gateway_timeout?(e) } then 'GATEWAY_TIMEOUT' + when ->(e) { server_timeout?(e) } then ServiceUnavailableError::CODE + when ->(e) { gateway_timeout?(e) } then GatewayTimeoutError::CODE else error.respond_to?(:code) ? error.code : INTERNAL_ERROR_CODE end end diff --git a/app/web/request/rate_limiter.rb b/app/web/request/rate_limiter.rb index 9b49cd60..5febcf8e 100644 --- a/app/web/request/rate_limiter.rb +++ b/app/web/request/rate_limiter.rb @@ -200,8 +200,14 @@ def handle_overflow(now) { size: post_prune_size, action: 'prune_to_limit' } ) - keys_to_evict = @history.keys.sample(post_prune_size - 10_000) - keys_to_evict.each { |k| @history.delete(k) } + needed = post_prune_size - 10_000 + evicted = 0 + @history.each_key do |key| + break if evicted >= needed + + @history.delete(key) + evicted += 1 + end end # rubocop:enable Metrics/MethodLength diff --git a/spec/html2rss/web/error_responder_spec.rb b/spec/html2rss/web/error_responder_spec.rb index b3048c42..65964979 100644 --- a/spec/html2rss/web/error_responder_spec.rb +++ b/spec/html2rss/web/error_responder_spec.rb @@ -150,6 +150,32 @@ def expected_api_error_response # rubocop:disable Metrics/MethodLength expect(response.status).to eq(504) expect(response['Retry-After']).to eq('300') end + + it 'preserves existing Retry-After header for 503 errors', :aggregate_failures do + existing_response = Rack::Response.new + existing_response['Retry-After'] = '10' + response, _body = respond_with( + error: Html2rss::Web::ServiceUnavailableError.new('service down'), + path: '/api/v1/feeds', + target: Html2rss::Web::RequestTarget::API, + response: existing_response + ) + expect(response.status).to eq(503) + expect(response['Retry-After']).to eq('10') + end + + it 'preserves existing Retry-After header for 504 errors', :aggregate_failures do + existing_response = Rack::Response.new + existing_response['Retry-After'] = '10' + response, _body = respond_with( + error: Html2rss::Web::GatewayTimeoutError.new('gateway down'), + path: '/api/v1/feeds', + target: Html2rss::Web::RequestTarget::API, + response: existing_response + ) + expect(response.status).to eq(504) + expect(response['Retry-After']).to eq('10') + end end # @return [Hash{String=>Object}] From 27c8d2cdb04687151f672fa3eaddf2255d828774 Mon Sep 17 00:00:00 2001 From: Gil Desmarais Date: Fri, 12 Jun 2026 13:05:24 +0200 Subject: [PATCH 2/4] fix: restore randomness to rate limiter eviction logic and add verification test --- app/web/request/rate_limiter.rb | 10 ++-------- spec/html2rss/web/rate_limiter_spec.rb | 22 ++++++++++++++++++++++ 2 files changed, 24 insertions(+), 8 deletions(-) diff --git a/app/web/request/rate_limiter.rb b/app/web/request/rate_limiter.rb index 5febcf8e..9b49cd60 100644 --- a/app/web/request/rate_limiter.rb +++ b/app/web/request/rate_limiter.rb @@ -200,14 +200,8 @@ def handle_overflow(now) { size: post_prune_size, action: 'prune_to_limit' } ) - needed = post_prune_size - 10_000 - evicted = 0 - @history.each_key do |key| - break if evicted >= needed - - @history.delete(key) - evicted += 1 - end + keys_to_evict = @history.keys.sample(post_prune_size - 10_000) + keys_to_evict.each { |k| @history.delete(k) } end # rubocop:enable Metrics/MethodLength diff --git a/spec/html2rss/web/rate_limiter_spec.rb b/spec/html2rss/web/rate_limiter_spec.rb index 75beee3a..fae6de4e 100644 --- a/spec/html2rss/web/rate_limiter_spec.rb +++ b/spec/html2rss/web/rate_limiter_spec.rb @@ -108,6 +108,28 @@ # Should prune down to 10,000 plus the active client request track = 10,001 expect(history_map.size).to eq(10_001) end + + it 'evicts keys randomly on overflow' do + history_map = middleware.instance_variable_get(:@history) + now = Time.now.to_i + + # Helper to fill history and trigger overflow + perform_overflow_eviction = lambda do + history_map.clear + 20_100.times do |i| + track = described_class::RequestTrack.new + track.instance_variable_set(:@timestamps, [now]) + history_map["ip-#{i}"] = track + end + middleware.send(:handle_overflow, now) + history_map.keys.sort + end + + remaining_keys_1 = perform_overflow_eviction.call + remaining_keys_2 = perform_overflow_eviction.call + + expect(remaining_keys_1).not_to eq(remaining_keys_2) + end # rubocop:enable RSpec/ExampleLength it 'logs rate limit exceeded events to SecurityLogger' do From bf3a6d60cb9cb53f2e83c86e11578cac2a15cf8b Mon Sep 17 00:00:00 2001 From: Gil Desmarais Date: Fri, 12 Jun 2026 13:07:04 +0200 Subject: [PATCH 3/4] fix: refine eviction logic for better concurrency and randomness --- app/web/request/rate_limiter.rb | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/app/web/request/rate_limiter.rb b/app/web/request/rate_limiter.rb index 9b49cd60..630725f1 100644 --- a/app/web/request/rate_limiter.rb +++ b/app/web/request/rate_limiter.rb @@ -200,8 +200,13 @@ def handle_overflow(now) { size: post_prune_size, action: 'prune_to_limit' } ) - keys_to_evict = @history.keys.sample(post_prune_size - 10_000) - keys_to_evict.each { |k| @history.delete(k) } + needed = post_prune_size - 10_000 + evicted = 0 + @history.keys.shuffle.each do |key| + break if evicted >= needed + + evicted += 1 if @history.delete(key) + end end # rubocop:enable Metrics/MethodLength From f7691c30f4666809755abd6ed568f13c0b69702b Mon Sep 17 00:00:00 2001 From: Gil Desmarais Date: Fri, 12 Jun 2026 13:23:32 +0200 Subject: [PATCH 4/4] style: silence rubocop --- spec/html2rss/web/rate_limiter_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/html2rss/web/rate_limiter_spec.rb b/spec/html2rss/web/rate_limiter_spec.rb index fae6de4e..7532049a 100644 --- a/spec/html2rss/web/rate_limiter_spec.rb +++ b/spec/html2rss/web/rate_limiter_spec.rb @@ -124,10 +124,10 @@ middleware.send(:handle_overflow, now) history_map.keys.sort end - + # rubocop:disable Naming/VariableNumber remaining_keys_1 = perform_overflow_eviction.call remaining_keys_2 = perform_overflow_eviction.call - + # rubocop:enable Naming/VariableNumber expect(remaining_keys_1).not_to eq(remaining_keys_2) end # rubocop:enable RSpec/ExampleLength