Skip to content

Avoid duplicating ActiveJob's retry machinery #11061

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
vrajmohan merged 1 commit into from
Aug 12, 2024
Merged

Avoid duplicating ActiveJob's retry machinery #11061

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 18 additions & 24 deletions app/jobs/risc_delivery_job.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down
170 changes: 118 additions & 52 deletions spec/jobs/risc_delivery_job_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Comment thread
Sgtpluck marked this conversation as resolved.
Outdated
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(
Expand All @@ -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)
Comment on lines 153 to 154

@zachmargolis zachmargolis Aug 9, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a way to use webmock to return the right error instead of manually stubbing Faraday?

@vrajmohan vrajmohan Aug 9, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIANM, the comments on lines 114-115 were from you and seemed to indicate that it would not simulate the real world behavior. Should I investigate further?

@zachmargolis zachmargolis Aug 9, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LOL I had completely forgotten, you can investigate more if you want, but it's probably fine as is

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

@Sgtpluck Sgtpluck Aug 9, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't personally really like shared_examples, but since you've set them up, why wouldn't you use them for the different types of errors?

@vrajmohan vrajmohan Aug 9, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My preference has been to use shared_examples for larger examples.

@Sgtpluck Sgtpluck Aug 9, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since they are designed for reuse, it is confusing to me that you would add them and then not use them for all the similar specs -- it makes me think these are different in some way. am i missing something about them that makes them different?

i would prefer to either write out the longer specs explicitly (my actual preference, since i find shared examples hard to read, especially when there are failures), or use them for all examples because of that confusion it creates. does that make sense?

@vrajmohan vrajmohan Aug 9, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what you mean. The mocking fixture was different, the call count expectation was different and the risc event payload error message was different. I'll get rid of the shared_examples as they seem to add to the confusion in this case.

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(
Expand Down Expand Up @@ -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(
Comment thread
vrajmohan marked this conversation as resolved.
Outdated
: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
Expand All @@ -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).
Expand Down