Skip to content
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

Fix when resque failure backend is already multiple #803

Merged
merged 3 commits into from
Dec 15, 2023

Conversation

sj26
Copy link
Contributor

@sj26 sj26 commented Nov 15, 2023

Goal

We had an outage today after recently introducing Bugsnag into our codebase because our Resque failure backend was already Resque::Failure::Multiple, which this code looks like it handles, except for a small mistake with the operator:

Resque::Failure::Multiple < Resque::Failure::Multiple
# => false

Resque::Failure::Multiple <= Resque::Failure::Multiple
# => true

The backend is likely to be the Resque::Failure::Multiple class, not a sub-class.

This meant that when the bugsnag instrumentation code ran and we ended up with:

Resque::Failure.backend
# => Resque::Failure::Multiple
Resque::Failure::Multiple.classes
# => [Resque::Failure::Redis, Bugsnag::Resque, Resque::Failure::Multiple]

So a failure was reported to Redis, then Bugsnag, then Redis, then Bugsnag, then Redis, then Bugsnag, and so on, until we got a "stack overflow" error.

For us, we had a worker which needed cleanup during resque boot, which involves reporting a failure, so none of our resque workers would boot or process work, resulting in an outage of all background queue processing.

Testing

The tests here are pretty literal and testing the implementation more than the outcome. It might be possible to refactor them to test the Resque side of things a little more, but that's a little more than I'd like to chew off in this PR.

We are currently using BUGSNAG_DISABLE_AUTOCONFIGURE to work around, and would love to get this change merged and released quickly so we can use Bugsnag error reporting for Resque 🙏

@clr182
Copy link

clr182 commented Nov 15, 2023

Hi @sj26

Thank you for providing this PR. I've added this to our backlog and will review this when priorities allow.

@clr182 clr182 added bug Confirmed bug backlog We hope to fix this feature/bug in the future labels Nov 15, 2023
@sj26
Copy link
Contributor Author

sj26 commented Dec 12, 2023

This bit us again today.

Could you please take a look at this. It's quite a small fix, and shouldn't take much of your time.

@mclack
Copy link

mclack commented Dec 14, 2023

Hi @sj26

Just a heads up that this is on our list to look at. I still can't give an ETA on when it'll be fully assessed and tested, but we'll make sure to provide any updates on this thread.

Thanks for your patience in the meantime.

@imjoehaines imjoehaines changed the base branch from master to next December 15, 2023 11:34
skip-checks: true
Copy link
Contributor

@imjoehaines imjoehaines left a comment

Choose a reason for hiding this comment

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

Thanks @sj26!

@imjoehaines imjoehaines merged commit a219d78 into bugsnag:next Dec 15, 2023
@imjoehaines imjoehaines mentioned this pull request Jan 8, 2024
@mclack mclack added released This feature/bug fix has been released and removed backlog We hope to fix this feature/bug in the future labels Jan 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed bug released This feature/bug fix has been released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants