From 15074776e039cfe25208b0daa42e0a0590f0936f Mon Sep 17 00:00:00 2001
From: Mia Sinek <mia@miasinek.com>
Date: Thu, 14 Nov 2024 17:15:33 +0100
Subject: [PATCH 1/3] WIP

---
 lib/userlist.rb                               |  6 ++
 lib/userlist/push/client.rb                   | 77 +++++++++++++++++--
 .../push/strategies/active_job/worker.rb      |  2 -
 lib/userlist/push/strategies/direct.rb        | 10 +--
 lib/userlist/retryable.rb                     |  4 +-
 spec/userlist/push/client_spec.rb             | 25 ++++++
 .../push/strategies/active_job/worker_spec.rb | 10 +--
 .../push/strategies/active_job_spec.rb        |  2 +
 spec/userlist/push/strategies/direct_spec.rb  | 19 ++---
 9 files changed, 116 insertions(+), 39 deletions(-)

diff --git a/lib/userlist.rb b/lib/userlist.rb
index 55f8f05..2e81e71 100644
--- a/lib/userlist.rb
+++ b/lib/userlist.rb
@@ -12,6 +12,12 @@ 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
 
diff --git a/lib/userlist/push/client.rb b/lib/userlist/push/client.rb
index 7b0ad96..8d392e7 100644
--- a/lib/userlist/push/client.rb
+++ b/lib/userlist/push/client.rb
@@ -8,7 +8,7 @@ class Push
     class Client
       include Userlist::Logging
 
-      def initialize(config = {})
+      def initialize(config = {})        
         @config = Userlist.config.merge(config)
 
         raise Userlist::ConfigurationError, :push_key unless @config.push_key
@@ -33,7 +33,7 @@ def delete(endpoint, payload = nil)
 
     private
 
-      attr_reader :config
+      attr_reader :config, :status, :last_error
 
       def http
         @http ||= begin
@@ -49,19 +49,54 @@ 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 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
+      end
 
-        http.start unless http.started?
-        response = http.request(request)
+      def retry?(error)
+        error.is_a?(Userlist::ServerError) || 
+          error.is_a?(Userlist::TooManyRequestsError) ||
+          error.is_a?(Userlist::TimeoutError)
+      end
 
-        logger.debug "Recieved #{response.code} #{response.message} with body #{response.body}"
+      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
 
@@ -72,6 +107,34 @@ def endpoint
       def token
         config.push_key
       end
+
+      def retryable
+        @retryable ||= Userlist::Retryable.new do |response|
+          @status = response.code.to_i
+      
+          error?
+        end
+      end
+
+      def ok?
+        status.between?(200, 299)
+      end
+
+      def error?
+        server_error? || rate_limited? || timeout?
+      end
+      
+      def server_error?
+        status.between?(500, 599)
+      end
+      
+      def rate_limited?
+        status == 429
+      end
+
+      def timeout?
+        status == 408
+      end
     end
   end
-end
+end
\ No newline at end of file
diff --git a/lib/userlist/push/strategies/active_job/worker.rb b/lib/userlist/push/strategies/active_job/worker.rb
index d4684be..dce1bda 100644
--- a/lib/userlist/push/strategies/active_job/worker.rb
+++ b/lib/userlist/push/strategies/active_job/worker.rb
@@ -5,8 +5,6 @@ class Push
     module Strategies
       class ActiveJob
         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/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..bcec026 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..e3db970 100644
--- a/spec/userlist/push/strategies/active_job/worker_spec.rb
+++ b/spec/userlist/push/strategies/active_job/worker_spec.rb
@@ -30,10 +30,10 @@
     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)
+  # 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
+  #   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

From e3a7c6158fb45e7202da396d3316299b4b25ed10 Mon Sep 17 00:00:00 2001
From: Mia Sinek <mia@miasinek.com>
Date: Fri, 15 Nov 2024 11:23:35 +0100
Subject: [PATCH 2/3] Enhance error handling

---
 lib/userlist.rb                               |   6 +-
 lib/userlist/push/client.rb                   | 100 +++++++++++-------
 .../push/strategies/active_job/worker.rb      |   5 +
 .../push/strategies/sidekiq/worker.rb         |   7 ++
 .../push/strategies/threaded/worker.rb        |  20 ++--
 spec/userlist/push/client_spec.rb             |   6 +-
 .../push/strategies/active_job/worker_spec.rb |   7 --
 .../push/strategies/threaded/worker_spec.rb   |   9 --
 8 files changed, 86 insertions(+), 74 deletions(-)

diff --git a/lib/userlist.rb b/lib/userlist.rb
index 2e81e71..d7e1086 100644
--- a/lib/userlist.rb
+++ b/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:
diff --git a/lib/userlist/push/client.rb b/lib/userlist/push/client.rb
index 8d392e7..53688be 100644
--- a/lib/userlist/push/client.rb
+++ b/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
\ No newline at end of file
+end
diff --git a/lib/userlist/push/strategies/active_job/worker.rb b/lib/userlist/push/strategies/active_job/worker.rb
index dce1bda..c1594fb 100644
--- a/lib/userlist/push/strategies/active_job/worker.rb
+++ b/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
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/spec/userlist/push/client_spec.rb b/spec/userlist/push/client_spec.rb
index bcec026..351d4b7 100644
--- a/spec/userlist/push/client_spec.rb
+++ b/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
 
diff --git a/spec/userlist/push/strategies/active_job/worker_spec.rb b/spec/userlist/push/strategies/active_job/worker_spec.rb
index e3db970..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)
-
-  #   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/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])

From f0d4b1ac270d2e349f81b1c10eb2a5d9aede7a7f Mon Sep 17 00:00:00 2001
From: Mia Sinek <mia@miasinek.com>
Date: Fri, 15 Nov 2024 12:55:03 +0100
Subject: [PATCH 3/3] Normalize HTTP status ranges for consistency

---
 lib/userlist/push/client.rb | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/lib/userlist/push/client.rb b/lib/userlist/push/client.rb
index 53688be..9a9f7c2 100644
--- a/lib/userlist/push/client.rb
+++ b/lib/userlist/push/client.rb
@@ -9,10 +9,10 @@ class Client
       include Userlist::Logging
 
       HTTP_STATUS = {
-        ok: (200..299),
-        server_error: (500..599),
-        timeout: 408,
-        rate_limit: 429
+        ok: 200..299,
+        server_error: 500..599,
+        timeout: 408..408,
+        rate_limit: 429..429
       }.freeze
 
       def initialize(config = {})
@@ -102,7 +102,7 @@ def error_message_for_status(status)
       end
 
       def status_type(status)
-        HTTP_STATUS.find { |type, range| range === status }&.first
+        HTTP_STATUS.find { |_, range| range.include?(status) }&.first
       end
 
       def error_mapping