diff --git a/lib/userlist.rb b/lib/userlist.rb index 55f8f05..d7e1086 100644 --- a/lib/userlist.rb +++ b/lib/userlist.rb @@ -9,8 +9,10 @@ module Userlist class Error < StandardError; end - class ArgumentError < Error; end + class ServerError < Error; end + class TooManyRequestsError < Error; end + class TimeoutError < Error; end class ConfigurationError < Error attr_reader :key @@ -18,7 +20,7 @@ class ConfigurationError < Error def initialize(key) @key = key.to_sym - super <<~MESSAGE + super(<<~MESSAGE) Missing required configuration value for `#{key}` Please set a value for `#{key}` using an environment variable: diff --git a/lib/userlist/push/client.rb b/lib/userlist/push/client.rb index 7b0ad96..9a9f7c2 100644 --- a/lib/userlist/push/client.rb +++ b/lib/userlist/push/client.rb @@ -8,6 +8,13 @@ class Push class Client include Userlist::Logging + HTTP_STATUS = { + ok: 200..299, + server_error: 500..599, + timeout: 408..408, + rate_limit: 429..429 + }.freeze + def initialize(config = {}) @config = Userlist.config.merge(config) @@ -33,7 +40,7 @@ def delete(endpoint, payload = nil) private - attr_reader :config + attr_reader :config, :status def http @http ||= begin @@ -49,19 +56,93 @@ def http end def request(method, path, payload = nil) + request = build_request(method, path, payload) + + log_request(request) + + http.start unless http.started? + + response = retryable.attempt do + response = http.request(request) + log_response(response) + end + + handle_response(response) + end + + def build_request(method, path, payload) request = method.new(path) request['Accept'] = 'application/json' request['Authorization'] = "Push #{token}" request['Content-Type'] = 'application/json; charset=UTF-8' request.body = JSON.generate(payload) if payload + request + end - logger.debug "Sending #{request.method} to #{URI.join(endpoint, request.path)} with body #{request.body}" + def handle_response(response) + @status = response.code.to_i + return response if ok? - http.start unless http.started? - response = http.request(request) + raise_error_for_status(status) + end - logger.debug "Recieved #{response.code} #{response.message} with body #{response.body}" + def raise_error_for_status(status) + error_class = error_class_for_status(status) + message = error_message_for_status(status) + raise error_class, message + end + + def error_class_for_status(status) + error_mapping[status_type(status)] || Userlist::ServerError + end + + def error_message_for_status(status) + message = error_messages[status_type(status)] || 'HTTP error' + "#{message}: #{status}" + end + + def status_type(status) + HTTP_STATUS.find { |_, range| range.include?(status) }&.first + end + + def error_mapping + { + server_error: Userlist::ServerError, + timeout: Userlist::TimeoutError, + rate_limit: Userlist::TooManyRequestsError + } + end + + def error_messages + { + server_error: 'Server error', + timeout: 'Request timeout', + rate_limit: 'Rate limit exceeded' + } + end + + def ok? + HTTP_STATUS[:ok].include?(status) + end + + def error? + [:server_error, :rate_limit, :timeout].include?(status_type(status)) + end + + def retryable + @retryable ||= Userlist::Retryable.new do |response| + @status = response.code.to_i + + error? + end + end + + def log_request(request) + logger.debug "Sending #{request.method} to #{URI.join(endpoint, request.path)} with body #{request.body}" + end + def log_response(response) + logger.debug "Received #{response.code} #{response.message} with body #{response.body}" response end diff --git a/lib/userlist/push/strategies/active_job/worker.rb b/lib/userlist/push/strategies/active_job/worker.rb index d4684be..c1594fb 100644 --- a/lib/userlist/push/strategies/active_job/worker.rb +++ b/lib/userlist/push/strategies/active_job/worker.rb @@ -4,9 +4,12 @@ module Userlist class Push module Strategies class ActiveJob + # Worker processes requests through ActiveJob. + # It forwards requests to the Userlist::Push::Client which handles: + # - HTTP communication + # - Retries for failed requests (500s, timeouts, rate limits) + # - Error handling for HTTP-related issues class Worker < ::ActiveJob::Base - retry_on Timeout::Error, wait: :polynomially_longer, attempts: 10 - def perform(method, *args) client = Userlist::Push::Client.new client.public_send(method, *args) diff --git a/lib/userlist/push/strategies/direct.rb b/lib/userlist/push/strategies/direct.rb index adebc52..520eab3 100644 --- a/lib/userlist/push/strategies/direct.rb +++ b/lib/userlist/push/strategies/direct.rb @@ -7,7 +7,7 @@ def initialize(config = {}) end def call(*args) - retryable.attempt { client.public_send(*args) } + client.public_send(*args) end private @@ -17,14 +17,6 @@ def call(*args) def client @client ||= Userlist::Push::Client.new(config) end - - def retryable - @retryable ||= Userlist::Retryable.new do |response| - status = response.code.to_i - - status >= 500 || status == 429 - end - end end end end diff --git a/lib/userlist/push/strategies/sidekiq/worker.rb b/lib/userlist/push/strategies/sidekiq/worker.rb index 25e85ad..1a3e6a3 100644 --- a/lib/userlist/push/strategies/sidekiq/worker.rb +++ b/lib/userlist/push/strategies/sidekiq/worker.rb @@ -4,6 +4,13 @@ module Userlist class Push module Strategies class Sidekiq + # Worker processes requests through Sidekiq. + # It forwards requests to the Userlist::Push::Client which handles: + # - HTTP communication + # - Retries for failed requests (500s, timeouts, rate limits) + # - Error handling for HTTP-related issues + # + # Note: Sidekiq retries (if configured) are separate from HTTP request retries class Worker include ::Sidekiq::Worker diff --git a/lib/userlist/push/strategies/threaded/worker.rb b/lib/userlist/push/strategies/threaded/worker.rb index f0dad23..f64158d 100644 --- a/lib/userlist/push/strategies/threaded/worker.rb +++ b/lib/userlist/push/strategies/threaded/worker.rb @@ -2,6 +2,16 @@ module Userlist class Push module Strategies class Threaded + # Worker processes requests from a queue in a separate thread. + # It forwards requests to the Userlist::Push::Client which handles: + # - HTTP communication + # - Retries for failed requests (500s, timeouts, rate limits) + # - Error handling for HTTP-related issues + # + # The worker itself only handles fatal errors to: + # - Prevent the worker thread from crashing + # - Keep processing the queue + # - Log errors for debugging class Worker include Userlist::Logging @@ -22,7 +32,7 @@ def run method, *args = *queue.pop break if method == :stop - retryable.attempt { client.public_send(method, *args) } + client.public_send(method, *args) rescue StandardError => e logger.error "Failed to deliver payload: [#{e.class.name}] #{e.message}" end @@ -44,14 +54,6 @@ def stop def client @client ||= Userlist::Push::Client.new(config) end - - def retryable - @retryable ||= Userlist::Retryable.new do |response| - status = response.code.to_i - - status >= 500 || status == 429 - end - end end end end diff --git a/lib/userlist/retryable.rb b/lib/userlist/retryable.rb index fc45aeb..acbbaf2 100644 --- a/lib/userlist/retryable.rb +++ b/lib/userlist/retryable.rb @@ -20,6 +20,7 @@ def retry?(value) end def attempt + result = nil (0..@retries).each do |attempt| if attempt.positive? milliseconds = delay(attempt) @@ -33,8 +34,7 @@ def attempt end logger.debug 'Retries exhausted, giving up' - - nil + result end private diff --git a/spec/userlist/push/client_spec.rb b/spec/userlist/push/client_spec.rb index 991134c..351d4b7 100644 --- a/spec/userlist/push/client_spec.rb +++ b/spec/userlist/push/client_spec.rb @@ -80,6 +80,31 @@ subject.post('/events', payload) end + + it 'retries on server errors (500s), timeouts (408), and rate limits (429)' do + stub_request(:post, 'https://endpoint.test.local/events') + .to_return( + { status: 500 }, # First attempt fails with server error + { status: 408 }, # Second attempt fails with timeout + { status: 429 }, # Third attempt fails with rate limit + { status: 200 } # Fourth attempt succeeds + ) + + expect_any_instance_of(Userlist::Retryable).to receive(:sleep).exactly(3).times + + response = subject.post('/events', { foo: :bar }) + expect(response.code).to eq('200') + end + + it 'gives up after maximum retry attempts' do + stub_request(:post, 'https://endpoint.test.local/events') + .to_return(status: 500).times(11) # Initial attempt + 10 retries + + expect_any_instance_of(Userlist::Retryable).to receive(:sleep).exactly(10).times + + expect { subject.post('/events', { foo: :bar }) } + .to raise_error(Userlist::ServerError, 'Server error: 500') + end end describe '#put' do diff --git a/spec/userlist/push/strategies/active_job/worker_spec.rb b/spec/userlist/push/strategies/active_job/worker_spec.rb index 0a2a558..ca1fcd8 100644 --- a/spec/userlist/push/strategies/active_job/worker_spec.rb +++ b/spec/userlist/push/strategies/active_job/worker_spec.rb @@ -29,11 +29,4 @@ described_class.perform_now('post', '/user', payload) described_class.perform_now('delete', '/user/identifier') end - - it 'should retry the job on failure' do - allow(client).to receive(:post).and_raise(Timeout::Error) - - expect { described_class.perform_now('post', '/events', payload) } - .to change(described_class.queue_adapter.enqueued_jobs, :size).by(1) - end end diff --git a/spec/userlist/push/strategies/active_job_spec.rb b/spec/userlist/push/strategies/active_job_spec.rb index 87c70bf..f4f911c 100644 --- a/spec/userlist/push/strategies/active_job_spec.rb +++ b/spec/userlist/push/strategies/active_job_spec.rb @@ -70,4 +70,6 @@ expect(job['priority']).to eq(42) end end + + end diff --git a/spec/userlist/push/strategies/direct_spec.rb b/spec/userlist/push/strategies/direct_spec.rb index b2031dd..5d6a85f 100644 --- a/spec/userlist/push/strategies/direct_spec.rb +++ b/spec/userlist/push/strategies/direct_spec.rb @@ -3,11 +3,15 @@ require 'userlist/push/strategies/direct' RSpec.describe Userlist::Push::Strategies::Direct do - subject { described_class.new } + subject { described_class.new(config) } let(:client) { instance_double('Userlist::Push::Client') } + let(:config) do + Userlist::Config.new(push_key: 'test-push-key', push_endpoint: 'https://endpoint.test.local') + end before do + allow(Userlist::Push::Client).to receive(:new).and_return(client) allow(client).to receive(:post) { double(code: '202') } end @@ -19,23 +23,10 @@ end describe '#call' do - before do - allow(Userlist::Push::Client).to receive(:new).and_return(client) - end - it 'should forward the call to the client' do payload = { foo: :bar } expect(client).to receive(:post).with('/users', payload) subject.call(:post, '/users', payload) end - - it 'should retry failed responses' do - payload = { foo: :bar } - - expect(client).to receive(:post) { double(code: '500') }.exactly(11).times - expect_any_instance_of(Userlist::Retryable).to receive(:sleep).exactly(10).times - - subject.call(:post, '/users', payload) - end end end diff --git a/spec/userlist/push/strategies/threaded/worker_spec.rb b/spec/userlist/push/strategies/threaded/worker_spec.rb index 08a9728..0ac0fcf 100644 --- a/spec/userlist/push/strategies/threaded/worker_spec.rb +++ b/spec/userlist/push/strategies/threaded/worker_spec.rb @@ -42,15 +42,6 @@ queue.push([:delete, '/user/identifier']) end - it 'should retry failed responses' do - payload = { foo: :bar } - - expect(client).to receive(:post) { double(code: '500') }.exactly(11).times - expect_any_instance_of(Userlist::Retryable).to receive(:sleep).exactly(10).times - - queue.push([:post, '/users', payload]) - end - it 'should log failed requests' do allow(client).to receive(:post).and_raise(StandardError) queue.push([:post, '/events', payload])