Dashboard: Deleting a project cascades computations that time out

[benjaoming]

Users cannot delete projects with a lot of data or views and end up with 502s.

On the Dashboard's "Delete Project" page, we use Django's built-in DeleteView:

readthedocs.org/readthedocs/projects/views/private.py

Lines 167 to 180 in 38fca65

	class ProjectDelete(UpdateChangeReasonPostView, ProjectMixin, DeleteView):
	success_message = _('Project deleted')
	template_name = 'projects/project_delete.html'
	def get_context_data(self, **kwargs):
	context = super().get_context_data(**kwargs)
	context['is_superproject'] = (
	self.object.subprojects.all().exists()
	)
	return context
	def get_success_url(self):
	return reverse('projects_dashboard')

It will call .delete() on the object to delete and then Django will call .delete() on any referencing FK that has on_delete=models.CASCADE

This likely becomes an issue because of FKs in models with a lot of data, such as this:

readthedocs.org/readthedocs/analytics/models.py

Lines 55 to 63 in 38fca65

	class PageView(models.Model):
	"""PageView counts per day for a project, version, and path."""
	project = models.ForeignKey(
	Project,
	related_name='page_views',
	on_delete=models.CASCADE,
	)

The solution is likely to clean up some relations with a bulk delete query here:

readthedocs.org/readthedocs/projects/models.py

Lines 498 to 504 in 38fca65

	def delete(self, *args, **kwargs): # pylint: disable=arguments-differ
	from readthedocs.projects.tasks.utils import clean_project_resources
	# Remove extra resources
	clean_project_resources(self)
	super().delete(*args, **kwargs)

[ericholscher]

I think we probably just want to delete things in a celery task, and not in process. To do that properly we need a bit more thought on UX, but in general even a 1-2 minute delete is better than a 502 that doesn't work :)

[benjaoming]

@ericholscher I like that the action is atomic and gives direct feedback. I don't think we need a celery task for this, we just need to not hit the database once for every page visit that has been logged in analytics. We can probably wrap a db transaction around this.

As for celery tasks, the deletion of content and search indexes takes place async. Unfortunately, they are added BEFORE deleting the project, so the atomicity is a bit so-so.. would be best to delete this after the deletion of the project has gone well. The post_delete signal has access to a deferred object instance.

[humitos]

I'd propose to explore this idea:

• Use on_delete=models.SET_NULL for most of these relations where there are many objects involved
• Create a Celery tasks that performs a cleanup of these "orphaned" models that executes periodically and remove them

I'm assuming that SET_NULL is a lot faster than CASCADE since the later is "emulated" by Django (https://docs.djangoproject.com/en/4.1/ref/models/fields/#django.db.models.CASCADE), but I'm might be wrong here.

@ericholscher

but in general even a 1-2 minute delete

If the query takes more than 30s, we will need to somehow exclude it from our general limitation.

@benjaoming

It will call .delete() on the object to delete and then Django will call .delete() on any referencing FK that has on_delete=models.CASCADE

This is not 100% true. Django won't call .delete() on any referencing FK

Model.delete() isn’t called on related models, but the pre_delete and post_delete signals are sent for all deleted objects.

(from https://docs.djangoproject.com/en/4.1/ref/models/fields/#django.db.models.CASCADE)

[stsewd]

@ericholscher I like that the action is atomic and gives direct feedback. I don't think we need a celery task for this, we just need to not hit the database once for every page visit that has been logged in analytics. We can probably wrap a db transaction around this.

I'm +1 with this idea, just manually call project.page_views.all().delete() to do a bulk deletion before deleting the project itself.

[benjaoming]

I'm assuming that SET_NULL is a lot faster than CASCADE since the later is "emulated" by Django (https://docs.djangoproject.com/en/4.1/ref/models/fields/#django.db.models.CASCADE), but I'm might be wrong here.

@humitos I'm not sure that getting rid of a DB constraint here is a good idea since it opens up for a potentially unwanted state that we didn't design for. So then we'd have to go and check all the places where this FK is in use 🔍

[ericholscher]

@ericholscher I like that the action is atomic and gives direct feedback. I don't think we need a celery task for this, we just need to not hit the database once for every page visit that has been logged in analytics. We can probably wrap a db transaction around this.

I'm +1 with this idea, just manually call project.page_views.all().delete() to do a bulk deletion before deleting the project itself.

That seems like a good, easy first step. @benjaoming Want to take this on?

[benjaoming]

Yes, all good and well-defined, would love to fix it 👍

[benjaoming]

Just updated the title to @humitos's point that it's not necessarily DB calls that cause the timeouts, but I'll update the issue with the cause once identified.

[humitos]

This is probably hitting the same issue we had in #10446 where we needed to call a raw SQL query to avoid Django performing a SELECT query to trigger all the pre_/post_ signals.