it looks like this request is an AichatRequest. My recommendation would be to pass database ids rather than ActiveRecord objects. ActiveRecord objects are not in the list of supported argument types for jobs: https://edgeguides.rubyonrails.org/active_job_basics.html#supported-types-for-arguments

they don't give a great explanation, but my assumption is that an object may fail to be deserialized if it is dequeued by a different version of the code than was used to enqueue it.

I did notice later in those docs there is the option to add a serializer for your class, which would allow you to keep passing the request object directly, in case that sounds preferable to you.

Yeah, I originally had request ID, but then saw the docs below about how GlobalID allows passing ActiveRecord objects directly. Looks like as per the docs, this works with any class that mixes in GlobalID::Identification, which by default has been mixed into Active Record classes so we may not even need the serializer?

That being said, I don't have a strong preference either way - if it's better code style to pass in the ID and lookup the object inside the job, that's fine with me too.

oh, nice find! I missed that GlobalID included active record objects. if this is supported then I think it sounds great to pass the object directly.

I did notice later in those docs there is the option to add a serializer for your class, which would allow you to keep passing the request object directly, in case that sounds preferable to you.

sorry to potentially stir up a contentious topic, but if there's no code issue to be fixed in response to this then I would suggest not logging it to HoneyBadger. could it be logged to CloudWatch instead? If you're aware honeybadger isn't ideal, but none of the other alternatives meet your needs, then I think it is probably ok to keep logging there until firehose is fixed or replaced, but if that's the case it'd be nice to hear why HB is necessary in this case.

Good question :) this is copied from what we're currently doing in the non-ActiveJob version of this API. Was previously discussed in this PR and this thread. I think the decision more or less came down to, HB isn't ideal but we decided it logically made sense to treat profane responses as "errors" since that indicates that there's something wrong with our system prompt or other parameters. CloudWatch was considered as an alternative but as per these discussions it looks like there wasn't an easy way to implement it at the time?

Thank you for the context! Honey Badger seems like the best option, given that you want to be notified promptly and take action.

Just noting that this seems like a "failed job" scenario, but notifying honeybadger directly without reraising will circumvent our monitoring for the number of failed jobs added in #59982 .

I'd suggest reraising and just including all the info you're passing to HB as part of the new exception, but I'm certainly open to pushback.

@cat5inthecradle what do you think? I have to say I'm not totally clear on how we're planning to use the failed job monitoring, if we're going to alert on it whether it would be on a per-service basis, etc. there may even be a bigger conversation here about whether we should be using separate queues which would make it easier to monitor them separately.

That makes sense. Will raise an exception here with the original message and request context.

Yeah, this will treat the jobs as successful. Maybe it's fine if you only care if the job "successfully made an attempt". If we'd prefer it retry until successful or N times, then we should re-raise and make sure the failed job behavior is what we want.

I'm also not seeing where this reports the failure back to whatever is waiting. But I'm also in the car and not looking super closely.

@cat5inthecradle re: reporting back the failure, yep that will happen in another PR where a controller action sends back the status of the request and the response. The client will be responsible for handing that failure status and updating the UI accordingly.

Does re-raising the exception automatically retry the job? I don't think we'd always want to retry the job here since it might be a non-retryable error from SageMaker.

no, reraising the exception does not automatically retry the job, unless perhaps it causes a retry_on clause to be hit. I'm actually not sure what the ordering is for whether rescue_from or retry_on gets called first. in any case, I agree that we do not want to retry here without understanding the details. in response to the honeybadger error, you could consider adding detection and retry logic for a new specific case if needed.

Do you need these in setup since they're set in factories?

Oh good catch! Yep can probably remove these - I can do so in a future PR.

Nice testing!