diff --git a/app/jobs/risc_delivery_job.rb b/app/jobs/risc_delivery_job.rb index c48afb8c243..0598f9ff0bf 100644 --- a/app/jobs/risc_delivery_job.rb +++ b/app/jobs/risc_delivery_job.rb @@ -14,14 +14,28 @@ class RiscDeliveryJob < ApplicationJob *NETWORK_ERRORS, wait: :polynomially_longer, attempts: 2, - ) do |_job, _exception| - # Don't bubble up the exception when retries are exhausted + ) do |job, exception| + args = job.arguments.first + job.track_event( + success: false, + error: exception.message, + event_type: args[:event_type], + issuer: args[:issuer], + user: args[:user], + ) end retry_on RedisRateLimiter::LimitError, wait: :polynomially_longer, - attempts: 10 do |_job, _exception| - # Don't bubble up the exception when retries are exhausted + attempts: 10 do |job, exception| + args = job.arguments.first + job.track_event( + success: false, + error: exception.message, + event_type: args[:event_type], + issuer: args[:issuer], + user: args[:user], + ) end def self.warning_error_classes @@ -57,26 +71,6 @@ def perform( success: response.success?, user:, ) - rescue *NETWORK_ERRORS => err - raise err if self.executions < 2 - - track_event( - error: err.message, - event_type:, - issuer:, - success: false, - user:, - ) - rescue RedisRateLimiter::LimitError => err - raise err if self.executions < 10 - - track_event( - error: err.message, - event_type:, - issuer:, - success: false, - user:, - ) end def rate_limiter(url) diff --git a/spec/jobs/risc_delivery_job_spec.rb b/spec/jobs/risc_delivery_job_spec.rb index 562b8d0890b..faae3b8bbda 100644 --- a/spec/jobs/risc_delivery_job_spec.rb +++ b/spec/jobs/risc_delivery_job_spec.rb @@ -1,6 +1,7 @@ require 'rails_helper' RSpec.describe RiscDeliveryJob do + include ActiveJob::TestHelper around do |ex| REDIS_THROTTLE_POOL.with { |client| client.flushdb } ex.run @@ -36,8 +37,7 @@ before do allow(job).to receive(:analytics).and_return(job_analytics) - allow(job).to receive(:queue_adapter). - and_return(ActiveJob::QueueAdapters::GoodJobAdapter.new) + ActiveJob::Base.queue_adapter = :test end it 'POSTs the jwt to the given URL' do @@ -62,23 +62,78 @@ ) end - context 'SSL network errors' do + context 'when the job fails due to a Faraday::SSLError' do before do stub_request(:post, push_notification_url).to_raise(Faraday::SSLError) + allow_any_instance_of(described_class).to receive(:analytics).and_return(job_analytics) end - it 'raises and retries via ActiveJob' do - expect { perform }.to raise_error(Faraday::SSLError) - end + context 'when the job fails for the 1st time' do + it 'raises and retries via ActiveJob' do + expect { perform }.to raise_error(Faraday::SSLError) - context 'it has already failed twice' do - before do - allow(job).to receive(:executions).and_return 2 + expect(job_analytics).not_to have_logged_event( + :risc_security_event_pushed, + risc_event_payload.merge( + error: 'Exception from WebMock', + ), + ) end + end + context 'when the job fails past the configured retry attempts' do it 'logs an event' do - expect { perform }.to_not raise_error + perform_enqueued_jobs do + RiscDeliveryJob.perform_later( + push_notification_url: push_notification_url, + jwt: jwt, + event_type: event_type, + issuer: issuer, + ) + end + + expect(a_request(:post, push_notification_url)).to have_been_made.times(2) + expect(job_analytics).to have_logged_event( + :risc_security_event_pushed, + risc_event_payload.merge( + error: 'Exception from WebMock', + ), + ) + end + end + end + + context 'when the job fails due to a Faraday::ConnectionFailed' do + before do + stub_request(:post, push_notification_url).to_raise(Faraday::ConnectionFailed) + allow_any_instance_of(described_class).to receive(:analytics).and_return(job_analytics) + end + + context 'when the job fails for the 1st time' do + it 'raises and retries via ActiveJob' do + expect { perform }.to raise_error(Faraday::ConnectionFailed) + + expect(job_analytics).not_to have_logged_event( + :risc_security_event_pushed, + risc_event_payload.merge( + error: 'Exception from WebMock', + ), + ) + end + end + context 'when the job fails past the configured retry attempts' do + it 'logs an event' do + perform_enqueued_jobs do + RiscDeliveryJob.perform_later( + push_notification_url: push_notification_url, + jwt: jwt, + event_type: event_type, + issuer: issuer, + ) + end + + expect(a_request(:post, push_notification_url)).to have_been_made.times(2) expect(job_analytics).to have_logged_event( :risc_security_event_pushed, risc_event_payload.merge( @@ -89,25 +144,41 @@ end end - context 'Errno::ECONNREFUSED error' do + context 'when the job fails due to an Errno::ECONNREFUSED error' do before do + allow_any_instance_of(described_class).to receive(:analytics).and_return(job_analytics) # stub_request().to_raise wraps this in Faraday::ConnectionFailed, but # in actual usage, the original error is unwrapped - expect(job.faraday).to receive(:post).and_raise(Errno::ECONNREFUSED) + @connection = instance_double(Faraday::Connection) + allow(@connection).to receive(:post).and_raise(Errno::ECONNREFUSED) + allow(Faraday).to receive(:new).and_return(@connection) end - it 'raises and retries via ActiveJob' do - expect { perform }.to raise_error(Errno::ECONNREFUSED) - end + context 'when the job fails for the 1st time' do + it 'raises and retries via ActiveJob' do + expect { perform }.to raise_error(Errno::ECONNREFUSED) - context 'it has already failed twice' do - before do - allow(job).to receive(:executions).and_return 2 + expect(job_analytics).not_to have_logged_event( + :risc_security_event_pushed, + risc_event_payload.merge( + error: 'Connection refused', + ), + ) end + end + context 'when the job fails past the configured retry attempts' do it 'logs an event' do - expect { perform }.to_not raise_error - + perform_enqueued_jobs do + RiscDeliveryJob.perform_later( + push_notification_url: push_notification_url, + jwt: jwt, + event_type: event_type, + issuer: issuer, + ) + end + + expect(@connection).to have_received(:post).exactly(2) expect(job_analytics).to have_logged_event( :risc_security_event_pushed, risc_event_payload.merge( @@ -153,27 +224,43 @@ end end - context 'slow network errors' do + context 'when the job encounters rate limiting' do before do - stub_request(:post, push_notification_url).to_timeout + allow_any_instance_of(described_class).to receive(:analytics).and_return(job_analytics) + @redis_rate_limiter = instance_double(RedisRateLimiter) + allow(@redis_rate_limiter).to receive(:attempt!).and_raise(RedisRateLimiter::LimitError) + allow(RedisRateLimiter).to receive(:new).and_return(@redis_rate_limiter) end - it 'raises and retries via ActiveJob' do - expect { perform }.to raise_error(Faraday::ConnectionFailed) - end + context 'when the job fails for the 1st time' do + it 'raises and retries via ActiveJob' do + expect { perform }.to raise_error(RedisRateLimiter::LimitError) - context 'it has already failed twice' do - before do - allow(job).to receive(:executions).and_return 2 + expect(job_analytics).not_to have_logged_event( + :risc_security_event_pushed, + risc_event_payload.merge( + error: 'RedisRateLimiter::LimitError', + ), + ) end + end + context 'when the job fails past the configured retry attempts' do it 'logs an event' do - expect { perform }.to_not raise_error - + perform_enqueued_jobs do + RiscDeliveryJob.perform_later( + push_notification_url: push_notification_url, + jwt: jwt, + event_type: event_type, + issuer: issuer, + ) + end + + expect(@redis_rate_limiter).to have_received(:attempt!).exactly(10) expect(job_analytics).to have_logged_event( :risc_security_event_pushed, risc_event_payload.merge( - error: 'execution expired', + error: 'RedisRateLimiter::LimitError', ), ) end @@ -187,27 +274,6 @@ end end - it 'raises on rate limit errors (and retries via ActiveJob)' do - expect { perform }.to raise_error(RedisRateLimiter::LimitError) - end - - context 'it has already failed ten times' do - before do - allow(job).to receive(:executions).and_return 10 - end - - it 'logs an event' do - expect { perform }.to_not raise_error - - expect(job_analytics).to have_logged_event( - :risc_security_event_pushed, - risc_event_payload.merge( - error: 'rate limit for push-notification-https://push.example.gov has maxed out', - ), - ) - end - end - context 'when the rate limit is overridden' do before do allow(IdentityConfig.store).to receive(:risc_notifications_rate_limit_overrides).