Strip ActiveJob keys to prevent recursion #386
Conversation
Once an ActiveJob is queued, ActiveRecord references get serialized into some internal reserved keys, such as _aj_globalid. The problem is, if this job in turn gets queued back into ActiveJob with these magic reserved keys, ActiveJob will throw up and error. We want to capture these and mutate the keys so we can sanely report it. Fixes GH-384
|
From #384:
It seems like the better option is to disable background Raven event sending when inside of any given background worker (ActiveJob, Resque, Sidekiq, etc.) Especially when the only other option is to special-case the removal of ActiveJob keys. |
|
As far as the implementation in this PR, shouldn't this code be abstracted out to ActiveJob, since it's not Sidekiq that's causing the bug? |
| # e.g.: _aj_globalid -> _globalid | ||
| if key[0..3] == '_aj_' | ||
| key = key[3..-1] | ||
| end |
There was a problem hiding this comment.
Wouldn't it be better to remove the keys entirely? If the ActiveJob job to send this event to Sentry fails, the metadata will be overridden if it's re-queued, so at the very least it's not reliable data.
There was a problem hiding this comment.
I wasn't sure if it'd be safe to strip them entirely or not. That was my initial thought, but decided it'd probably be better to err on the side of preserving as much data as possible.
There was a problem hiding this comment.
@seanlinsley's right, this is internal private state that's likely wrong/unreliable anyway. We should just drop them.
This is probably true. I'll have to dig around and figure out how that works.
I dug into the I'm pretty new to ruby, and trying to get familiar with these things. :) |
I think I disagree. The bug is that we're creating an object in a forbidden way. That's our problem. |
| end | ||
| end | ||
|
|
||
| def filter_context_hash(key, value) |
There was a problem hiding this comment.
Is there any way we can access the RESERVED_KEYS constant directly here? (I think that's what it's called)
There was a problem hiding this comment.
That'll dedup the knowledge of what a "forbidden" key is.
There was a problem hiding this comment.
I wanted to, but it appears to be private.
https://github.com/rails/rails/blob/master/activejob/lib/active_job/arguments.rb#L130-L135
There was a problem hiding this comment.
So I err'd on the side of being liberal and catching _aj_ prefix rather than duping the list in here and worrying about maintaining that.
There was a problem hiding this comment.
I suppose the only truly maintainable solution in the long term would be to rescue SerializationErrors. Hm.
|
I think we're on the right track, but it should live in active_job.rb |
|
This looks great. Looking forward to deploying this in our app so we can send in the background again. Any idea when you're planning to have this merged and released? |
nateberkopec
force-pushed
the
master
branch
2 times, most recently
from
6809be8
to
cd2f174
Compare
It took me a little while to understand this, but the reason this code is in the Sidekiq integration is because we don't load the ActiveJob integration when using Activejob with Sidekiq. This is because the Sidekiq integration gives us better access to the Sidekiq So, this is bug happens when all of these conditions are met:
This bug doesn't happen with the ActiveJob integration because we're much more stingy with the data we pass along in that integration. The object with the |
|
Decided I couldn't think of a better solution, so better to merge a fix. Had to update for style: a96d47b |
|
:) thx homie |
|
Confirmed with the test app I created for the issue that this fix worked. What is the release ETA for this version of the gem? Would like to update and get it running on my apps. Thanks again guys! 👏 |
Once an ActiveJob is queued, ActiveRecord references get serialized into
some internal reserved keys, such as _aj_globalid.
The problem is, if this job in turn gets queued back into ActiveJob with
these magic reserved keys, ActiveJob will throw up and error. We want to
capture these and mutate the keys so we can sanely report it.
Fixes GH-384