Enhance error handling

lib/userlist.rb

@@ -9,13 +9,9 @@
 
 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
@@ -24,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:

lib/userlist/push/client.rb

@@ -8,7 +8,14 @@ class Push
     class Client
       include Userlist::Logging
 
-      def initialize(config = {})        
+      HTTP_STATUS = {
+        ok: (200..299),
+        server_error: (500..599),
+        timeout: 408,
+        rate_limit: 429
+      }.freeze
+
+      def initialize(config = {})
         @config = Userlist.config.merge(config)
 
         raise Userlist::ConfigurationError, :push_key unless @config.push_key
@@ -33,7 +40,7 @@ def delete(endpoint, payload = nil)
 
     private
 
-      attr_reader :config, :status, :last_error
+      attr_reader :config, :status
 
       def http
         @http ||= begin
@@ -60,7 +67,7 @@ def request(method, path, payload = nil)
           log_response(response)
         end
 
-        handle_response response
+        handle_response(response)
       end
 
       def build_request(method, path, payload)
@@ -73,68 +80,79 @@ def build_request(method, path, payload)
       end
 
       def handle_response(response)
-        status = response.code.to_i
-
-        return response if status.between?(200, 299)
-
-        case status
-        when 500..599 then raise Userlist::ServerError, "Server error: #{status}"
-        when 408 then raise Userlist::TimeoutError, 'Request timed out'
-        when 429 then raise Userlist::TooManyRequestsError, 'Rate limited'
-        else raise Userlist::Error, "HTTP #{status}: #{response.message}"
-        end
+        @status = response.code.to_i
+        return response if ok?
+
+        raise_error_for_status(status)
       end
 
-      def retry?(error)
-        error.is_a?(Userlist::ServerError) || 
-          error.is_a?(Userlist::TooManyRequestsError) ||
-          error.is_a?(Userlist::TimeoutError)
+      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 log_request(request)
-        logger.debug "Sending #{request.method} to #{URI.join(endpoint, request.path)} with body #{request.body}"
+      def error_class_for_status(status)
+        error_mapping[status_type(status)] || Userlist::ServerError
       end
 
-      def log_response(response)
-        logger.debug "Received #{response.code} #{response.message} with body #{response.body}"
-        response
+      def error_message_for_status(status)
+        message = error_messages[status_type(status)] || 'HTTP error'
+        "#{message}: #{status}"
       end
 
-      def endpoint
-        @endpoint ||= URI(config.push_endpoint)
+      def status_type(status)
+        HTTP_STATUS.find { |type, range| range === status }&.first
       end
 
-      def token
-        config.push_key
+      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 ok?
-        status.between?(200, 299)
+      def log_request(request)
+        logger.debug "Sending #{request.method} to #{URI.join(endpoint, request.path)} with body #{request.body}"
       end
 
-      def error?
-        server_error? || rate_limited? || timeout?
-      end
-
-      def server_error?
-        status.between?(500, 599)
+      def log_response(response)
+        logger.debug "Received #{response.code} #{response.message} with body #{response.body}"
+        response
       end
-      
-      def rate_limited?
-        status == 429
+
+      def endpoint
+        @endpoint ||= URI(config.push_endpoint)
       end
 
-      def timeout?
-        status == 408
+      def token
+        config.push_key
       end
     end
   end
-end
+end

lib/userlist/push/strategies/active_job/worker.rb

@@ -4,6 +4,11 @@ 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
           def perform(method, *args)
             client = Userlist::Push::Client.new

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

spec/userlist/push/client_spec.rb

@@ -91,7 +91,7 @@
         )
 
       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
@@ -101,9 +101,9 @@
         .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")
+        .to raise_error(Userlist::ServerError, 'Server error: 500')
     end
   end
 

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