Build: don't run_post_build_tasks in a task

[stsewd]

Two things here:

It's run as a side effect of saving an object using the API. It should have its own endpoint.

readthedocs.org/readthedocs/api/v2/views/model_views.py

Lines 264 to 277 in a9b1438

	def perform_update(self, serializer):
	"""
	Run the post-build tasks when an isolated build reaches a final state.
	"""
	# Read before saving: this is still the state stored in the database.
	was_finished = serializer.instance.finished
	build = serializer.save()
	if (
	not was_finished
	and build.finished
	and build.project.has_feature(Feature.USE_ISOLATED_BUILDER)
	):
	run_post_build_tasks.delay(build_pk=build.pk)

It's run as a task, but it doesn't have any blocking calls (everything blocking is called as a task inside it). We should make this a function or call the blocking calls directly.

readthedocs.org/readthedocs/builds/tasks.py

Line 705 in a9b1438

def run_post_build_tasks(build_pk):

[humitos]

I'd like to combine this with the DAU approach and have only one way to end a build and start processing it. Either

1. calling the same /complete endpoint to finalize a build
2. using a post_save(model=Build) signal to perform this automatically on the backend app

I think that 1) is more explicit here, but probably not needed. I think we can do this automatically from the backend app using something like 2) and don't depend on the client to communicate the build has completed. They are already communicating it when sending the FINISHED status 👍🏼

What do you think?

[stsewd]

The problem with a signal is that when an old build object is saved, the signal would be triggered, and we don't want old builds to change the state of a version, or having to take that into consideration everytime we save a build.

[humitos]

Yeah. The idea is to limit when the signal is executed. Something like this.

if build.from_state != FINISHED and build.to_state == FINISHED and build.date >= 3 hours ago and something else:
  run_post_build_tasks

We can add as many checks as we want here to prevent triggering the post build tasks if it's not the correct moment.

[stsewd]

that feels like a hidden/unexpected side effect, having an explicit call after the build finishes looks better.

[humitos]

Hrm, it looks like the normal flow of a build to me. When the build finishes, the post_build_tasks are ran. My point is that the caller is already telling us that the build is finished; why we would need to re-confirm the build is finished by hitting another endpoint from the caller again to trigger this post_build_tasks?

[stsewd]

Hrm, it looks like the normal flow of a build to me. When the build finishes, the post_build_tasks are ran.

You are checking for the date and other heuristics to make sure the build was marked as finished after the build was finished. There isn't an explicit caller doing the call, the signal is fired anytime the build is saved. Don't think post save signals have access to the previous state of the object.