ExponentialBackoffRetry crashes if message was already deleted

[felixbuenemann]

If a worker that has the retry_intervals options calls sqs_msg.delete and then crashes after that, the exponential backoff handler crashes because it tries to set the visibility timeout of the non-existant message which raises:

#<Aws::SQS::Errors::InvalidParameterValue: Value AQEB…VJ for parameter ReceiptHandle is invalid. Reason: Message does not exist or is not available for visibility timeout change.>

One possible workaround in ExponentialBackoffRetry#handle_failure:

begin
  sqs_msg.change_visibility(visibility_timeout: interval.to_i)
rescue Aws::SQS::Errors::InvalidParameterValue => e
  message_not_found = "Message does not exist or is not available for visibility timeout change."
  raise unless e.message.end_with? message_not_found
else
  logger.info { "Message #{sqs_msg.message_id} failed, will be retried in #{interval} seconds." }
end

I looked at the sqs_msg, but it doesn't appear to have any property to detect that is has been deleted.

A more involved method would be to wrap or extend the sqs message instance passed into the worker/middleware with something that keeps track of deletion.

[felixbuenemann]

Hmm, the above mentioned solution is not good, as it still hides the original error. So I guess if the message is deleted we should raise the original error instead (which would confusingly be in e.cause at that point.

[felixbuenemann]

This works, but can probably improved to be more readable:

begin
  original_error = $!
  sqs_msg.change_visibility(visibility_timeout: interval.to_i)
rescue Aws::SQS::Errors::InvalidParameterValue => e
  message_not_found = "Message does not exist or is not available for visibility timeout change."
  if e.message.end_with? message_not_found
    raise original_error
  else
    raise
  end
else
  logger.info { "Message #{sqs_msg.message_id} failed, will be retried in #{interval} seconds." }
end

[phstc]

@felixbuenemann not sure if ExponentialBackoffRetry should treat this. The recommendation is to delete the messages as the last thing a worker would do, or using auto_delete: true (which will auto delete at end). Is there a specific reason for you deleting the message before finishing processing?

[felixbuenemann]

Yeah, I have code where there's network code at the beginning that is prone to errors (eg. S3 node returning 500-Error) and then just before it starts processing the downloaded data I delete the message, because if the following code crashes it is very unlikely that a retry would help, so I just want to report the exception to sentry (I've created a middleware for that) and give up.

[phstc]

@felixbuenemann got it. Probably this #329, can help you:

class MyWorker
  include Shoryuken::Worker

  shoryuken_options queue: 'default', 
    retry_intervals: ->(attempts) { method_that_returns_nil_when_the_worker_failed }
end

The exponential won't retry if the value is nil.