Enhance error handling messages

lib/userlist.rb

@@ -9,16 +9,18 @@
 
 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
 
     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:

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
 

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)

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

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
 

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

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

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

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

spec/userlist/push/strategies/active_job_spec.rb

@@ -70,4 +70,6 @@
       expect(job['priority']).to eq(42)
     end
   end
+
+
 end

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

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])