Throw an exception from Celery retry()

[agjohnson]

Without an exception here, or handling the return from
_check_concurrency_limit, a build that hits a concurrency limit in
this method will both set a retry on the task, but will also continue
processing the current task like normal. This results in two identical
builds, then three, then four, and so on.

Untested what this does to the UI though. Edit: the UI reports the retry as I'd expect 👍

[agjohnson]

cc @humitos as you might have more background on why throws=False might be required here.

[ericholscher]

I'm guessing this is just copied from before, where we were not handling exceptions nicely, and were explicitly returning False after this line.

[humitos]

This looks good for now as a quick fix and it seems that solved the problem, so 🎉

However, I just wanted to note here that I need to check a little deeper the workflow to understand what's going on:

• when calling self.retry(throw=True), does it go to on_retry or to on_failure?
  • If it goes to on_failure we will be sending a notification when we shouldn't
  • inside on_failure we are not populating the {limit} variable from the error user message
• what happens with the autoretry_for attribute from the class at

  readthedocs.org/readthedocs/projects/tasks/builds.py

  Lines 213 to 215 in 26e371b

  	autoretry_for = (
  	BuildMaxConcurrencyError,
  	)

  ? is it working?

I'm happy merging this, but I want to come back here and do some tests to understand exactly what's happening so we can document it properly and execute the exact code we want for each case.

[humitos]

Also, I saw that there is still a problem with the concurrency limit still: https://sentry.io/organizations/read-the-docs/issues/2998694596/ that I want to come back and double check.

[agjohnson]

Ahh yeah good catch, looks like this PR isn't the only fix we need here. Shall we merge this so it's not sitting around hotfix, and move the remaining pieces to a separate issue?

[humitos]

Yes. I'm merging this PR. The test issue seems unrelated.

[humitos]

when calling self.retry(throw=True), does it go to on_retry or to on_failure?

It goes to on_retry. We can check for isinstance(exc, BuildMaxConcurrencyError) and execute the code we need there (e.g. update the build object, etc). This way we will be handling all the retries in the same place.

what happens with the autoretry_for attribute from the class at

This works perfectly. However, to make usage of this attribute we need to raise BuildMaxConcurrencyError from inside the task instead of manually call self.retry. See https://docs.celeryproject.org/en/master/userguide/tasks.html#automatic-retry-for-known-exceptions

I'm opening a PR with these changes to make usage of these Celery features.