LG-13191 rescue faraday servererror

[svalexander]

🎫 Ticket

Link to the relevant ticket:
LG-13191

🛠 Summary of changes

This pr rescues Faraday::ServerError and logs to our event channel when this error is encountered when running the UspsAuthTokenRefreshJob. Making this change will keep us from alerting the #login-alarms channel when this error is encountered.

📜 Testing Plan

Provide a checklist of steps to confirm the changes.

• Specs run successfully

[zachmargolis]

is this to address the UspsAuthTokenRefreshJob errors we've been seeing? If so, that job does not reference this constant so it won't have any effect

[svalexander]

ok i think i;m missing something then. The def I found for warning_error_classes referenced NETWORK_ERRORS but i see now i didn't carefully check which job was using that. Where should i be looking instead?

[zachmargolis]

directly in that job:

identity-idp/app/jobs/usps_auth_token_refresh_job.rb

Line 10 in c7f7d2e

rescue Faraday::TimeoutError, Faraday::ConnectionFailed => err

[zachmargolis]

alternatively, the UspsAuthTokenRefreshJob could override its warning_error_classes and implement its own list, similar to the RiscDeliveryJob:

identity-idp/app/jobs/risc_delivery_job.rb

Lines 22 to 24 in c7f7d2e

	def self.warning_error_classes
	NETWORK_ERRORS + [RedisRateLimiter::LimitError]
	end

[zachmargolis]

I can't select it, but I'd expect the test above to error -- if we're catching a ServerError, this should fail:

        expect do
          subject.perform
        end.to raise_error

because there is no error being thrown

[svalexander]

i see what you're saying, it is passing though which is curious. I;m trying to figure out why this is happening.

[zachmargolis]

If you take off the expect { ... }.to_not raise_error you see that it's throwing an error for a method expectation based on a different logging method called.

so it's technically still throwing an error and the spec technically passes.

If the test was originally written to say expect { ... }.to_not raise_error(Faraday::ServerError) then it would have been a clearer failure here

[svalexander]

when i update the test to ...to_not raise_error(Faraday::ServerError) to check what happens it passes

[zachmargolis]

It passes with to_not raise_error(Faraday::ServerError) because gets this error instead from the stub

       #<InstanceDouble(Analytics) (anonymous)> received unexpected message :idv_usps_auth_token_refresh_job_network_error with ({:exception_class=>"Faraday::ServerError", :exception_message=>"the server responded with status 500"})

I pushed 8401797 to switch out the instance_double for a FakeAnalytics instance which is a better fake logger and has clearer spec failures when the rescue for Faraday::ServerError is removed

[svalexander]

ahh ok that makes sense! i think we still need the completion log, I'll add it in. Thanks for making this change!

[svalexander]

@zachmargolis I'm wondering how i can test this to confirm that #login-alarms wouldnt be alerted

[zachmargolis]

The alarms go off when the jobs have unhandled exceptions, so by handling the exception (and the subject.perform doesn't throw) then the alarms won't go off