Skip ActiveJob retrying for ProcessInstallmentsJob

[aldesantis]

Since ProcessInstallmentsJob is retried daily in any case, we should make sure to disable the default retry logic, to avoid creating tons of canceled orders in case something goes wrong.

[ikraamg]

Hi @aldesantis. I may not be understanding correctly, but if we remove the default retry_logic then I think we also don't need the clear_past_installments feature/configuration anymore as we won't have infinite retries.

Is this configuration also preventing the ProcessInstallmentsJob from retrying the failed installment? I suppose there might be a use case still if this is so. Can you please clarify this idea?

[aldesantis]

@ikraamg hey, unfortunately we'll still need clear_past_installments. There are basically two layers of retry logic at play in solidus_subscriptions:

1. ActiveJob will attempt to retry a background job that fails by delegating that to the underlying adapter (e.g., Sidekiq). This means that if an exception is raised during the execution of a ProcessInstallmentJob, that job will be retried with an exponential backoff.
2. If the background job doesn't raise an exception, but the installment can't be processed for some other reason (e.g., the user's credit card is declined), solidus_subscriptions also has its own retry mechanism that will retry the installment on the next day. It does this by moving the actionable_date to the next day and re-processing the installment through the Processor service object.

Note that the mechanism described in #2 will also cover any errors described in #1: in other words, if ProcessInstallmentJob raises an exception, the installment will be reprocessed through ActiveJob's native retry mechanism AND, if it still hasn't been processed successfully on the next day, also by solidus_subscriptions. This causes background jobs and cancelled orders to accumulate, and potentially a bad customer experience (e.g., because we might be sending multiple "Failed to process subscription" emails to customers).

This ticket is about disabling ActiveJob's native retry mechanism so that solidus_subscriptions can do its job of retrying failed installments every day until they are processed successfully.

clear_past_installments is still needed because it tells solidus_subscriptions that an installment should only be retried (via its own mechanism) up until the next subscription cycle: if my subscription fails processing today, and the next cycle processes in 7 days, I don't want to be charged for two cycles, just one.

[ikraamg]

@aldesantis Thanks for the detailed explanation! I see that I completely misunderstood what was required in this ticket.

In this case, I believe a simple discard_on is needed then 🤔 . Is it safe to discard_on StandardError. I ask because the the documentation indicates the during the perform method, an ActiveJob::DeserializationError will be raised if the record can't be deserialized but it might also raise a CustomAppException for something domain specific.

[aldesantis]

@ikraamg I think it’s safe to discard on StandardError, because the installment will be retried on the next day anyway.

We do need to make sure discard_on doesn’t prevent the exception from being raised and notified to services such as Sentry — we don’t want subscription processing to fail silently in production. 😄