-
-
Notifications
You must be signed in to change notification settings - Fork 511
Fix user login timeout error from bugsnag #6599
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.
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
Conversation
bf899cb to
a67a5f9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a production bug where password reset emails timing out with Net::ReadTimeout were causing crashes. The solution implements graceful timeout handling in the password reset flow, adds SMTP timeout configurations, and improves test coverage while fixing spelling errors.
Key changes:
- Added timeout error handling for email sending with fallback token generation for SMS
- Configured 5-second SMTP timeouts in production to prevent indefinite hangs
- Refactored tests to use
ActionMailer::Base.deliveriesinstead ofexpect_any_instance_ofand added comprehensive timeout scenario coverage
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| app/controllers/users/passwords_controller.rb | Added rescue block for Net::ReadTimeout and Net::OpenTimeout in send_password_reset_mail with logging, Bugsnag notification, and fallback token generation; fixed @resource initialization in invalid_phone_number_error |
| config/environments/production.rb | Added 5-second open_timeout and read_timeout to SMTP settings to prevent indefinite blocking |
| spec/requests/users/passwords_spec.rb | Fixed "twillio" → "twilio" spelling throughout; refactored email tests from expect_any_instance_of to ActionMailer::Base.deliveries; added comprehensive test coverage for timeout scenarios, phone validation, and password reset edge cases |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| it "generates a fallback token for SMS to use" do | ||
| expect(user).to receive(:generate_password_reset_token).and_call_original | ||
| request | ||
| end |
Copilot
AI
Nov 20, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test has a stubbing conflict. The before block at line 146 already stubs send_reset_password_instructions on the user object, but this test adds another expectation with receive on line 169. This second stub will override the first one, preventing send_reset_password_instructions from raising the Net::ReadTimeout exception that this test is supposed to verify handling for.
Consider moving the generate_password_reset_token expectation into the before block and chaining it:
allow(user).to receive(:send_reset_password_instructions).and_raise(Net::ReadTimeout)
allow(user).to receive(:generate_password_reset_token).and_call_originalOr use a separate test setup that doesn't conflict with the shared before block.
fixing issue seen in prod - see discord logs of errors from bugsnag - https://discord.com/channels/1145803216638984284/1145803694835773501/1438204066416164975
