Notifications: duplicate messages

[agjohnson]

Noticed here:
https://beta.readthedocs.org/projects/xclim/builds/23494705/

The error messages seem to stack here:

Also not sure why the string interpolation fails for these, I've noticed that in a few spots though.

On the current dashboard there are no notifications though, not sure why.

https://readthedocs.org/projects/xclim/builds/23494705/

[agjohnson]

cc @humitos I don't know if we've fixed this yet or not

[humitos]

Without jumping too much on this yet:

1. for some reason the build is not being reset properly when it's retried. We have code that cleanup all the notifications:

   readthedocs.org/readthedocs/builds/models.py

   Lines 1092 to 1109 in 0a381a7

   	def reset(self):
   	"""
   	Reset the build so it can be re-used when re-trying.
   	Dates and states are usually overridden by the build,
   	we care more about deleting the commands.
   	"""
   	self.state = BUILD_STATE_TRIGGERED
   	self.status = ''
   	self.success = True
   	self.output = ''
   	self.error = ''
   	self.exit_code = None
   	self.builder = ''
   	self.cold_storage = False
   	self.commands.all().delete()
   	self.notifications.all().delete()
   	self.save()

2. since the notification is exactly the same, it should be de-duplicated when it's added. However, when checking the APIv2 view (the one used by the builders), it doesn't seem we are using our custom .add() method that de-duplicates notifications:

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

   Lines 382 to 399 in 0a381a7

   	def perform_create(self, serializer):
   	"""Restrict creation to notifications attached to the project's builds from the api key."""
   	attached_to = serializer.validated_data["attached_to"]
   	build_api_key = self.request.build_api_key
   	project_slug = None
   	if isinstance(attached_to, Build):
   	project_slug = attached_to.project.slug
   	elif isinstance(attached_to, Project):
   	project_slug = attached_to.slug
   	# Limit the permissions to create a notification on this object only if the API key
   	# is attached to the related project
   	if not project_slug or build_api_key.project.slug != project_slug:
   	raise PermissionDenied()
   	return super().perform_create(serializer)

So, working on 2) to call .add() should be the first point to attack here. I suppose we should overwrite the api.v2.NotificationSerializer.save() method to call our custom .add().

[humitos]

On the current dashboard there are no notifications though, not sure why.
readthedocs.org/projects/xclim/builds/23494705

I think this is because we implemented permissions on notifications in the last deploy (see #11117) and there may be a bug on it because those notifications should be shown to all the users, since the build is public.

[humitos]

Also not sure why the string interpolation fails for these, I've noticed that in a few spots though.

In [12]: Notification.objects.filter(message_id=BuildMaxConcurrencyError.LIMIT_REACHED, format_values={}).count()
Out[12]: 4595

In [13]: Notification.objects.filter(message_id=BuildMaxConcurrencyError.LIMIT_REACHED).exclude(format_values={}).count()
Out[13]: 43746

It seems somewhere we are creating this notification without the proper format_values. 10% of them are broken. I suppose there should be a Celery workflow that we are not handling correctly here. I doubt it's this one in particular

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

Lines 362 to 374 in 0a381a7

	if concurrency_limit_reached:
	# By calling ``retry`` Celery will raise an exception and call ``on_retry``.
	# NOTE: autoretry_for doesn't work with exceptions raised from before_start,
	# it only works if they are raised from the run/execute method.
	log.info("Concurrency limit reached, retrying task.")
	self.retry(
	exc=BuildMaxConcurrencyError(
	BuildMaxConcurrencyError.LIMIT_REACHED,
	format_values={
	"limit": max_concurrent_builds,
	},
	)
	)

[agjohnson]

This is probably worth a separate issue, but I've noticed these build concurrency notifications on a number of builds too. Shouldn't these notifications clear after the build is successfully started?

I've noticed these in a few spots, but it's hard for me to tell if this should be resolved now or not. The above error is on my local instance on a finished build -- same with the original errors I posted here.

I suppose also these should be warning level notifications? That would at least help make it clear that the build is not failed.

[humitos]

I suppose also these should be warning level notifications? That would at least help make it clear that the build is not failed.

I've done this in #11196

[humitos]

Shouldn't these notifications clear after the build is successfully started?

Yes, I've mentioned this in 1) from my previous comment #11131 (comment)

[humitos]

So, working on 2) to call .add() should be the first point to attack here. I suppose we should overwrite the api.v2.NotificationSerializer.save() method to call our custom .add().

This is done in #11197

[humitos]

Now, with the two PRs I've opened I think this issue shouldn't be an issue anymore. However, 1) could be still present sometimes due to network issues/web instance congestion or similar scenarios where the reset API call fails to perform. That said, I'd would move forward with what we have keeping the issue open for now in case we continue seeing it but I wouldn't invest too much time on digging further on debugging for now.

[agjohnson]

Yes, I've mentioned this in 1) from my previous comment #11131 (comment)

That comment implied to me that the messages would only be deduplicated with the fix -- ie:

since the notification is exactly the same, it should be de-duplicated when it's added.

Are you saying that this fix is also what will dismiss the notification once the build has started?

To clarify, what I would expect here is that while the build is still queued that warning message should show, but once the build is no longer queued due to concurrency limits the message will disappear.

[humitos]

what I would expect here is that while the build is still queued that warning message should show, but once the build is no longer queued due to concurrency limits the message will disappear.

Yes, this is how it should work.

However, even with all these fixes... the scenario I mentioned before, may be still present on some edge cases 👇🏼

However, 1) could be still present sometimes due to network issues/web instance congestion or similar scenarios where the reset API call fails to perform.

[humitos]

I found this issue in https://readthedocs.org/projects/docs/builds/23722719/ as well. This was a recent build in our own documentation. I still don't understand why this is happening, but it's worth investigating it a little more.

[humitos]

We should probably run a small Python code from Django shell to remove these notifications from builds that are in finished state.

[humitos]

This would be Python code we could run in production to remove these invalid notifications from builds:

from readthedocs.builds.constants import (
    BUILD_STATE_FINISHED,
    BUILD_STATE_CANCELLED,
)
from readthedocs.doc_builder.exceptions import BuildMaxConcurrencyError


for n in Notification.objects.filter(message_id=BuildMaxConcurrencyError.LIMIT_REACHED).prefetch_related("attached_to"):
  if n.attached_to.state in (BUILD_STATE_FINISHED, BUILD_STATE_CANCELLED):
    n.delete()

[humitos]

I think I found why we are not resetting the build in some cases:

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

Lines 449 to 453 in a0526ff

	def _reset_build(self):
	# Reset build only if it has some commands already.
	if self.data.build.get("commands"):
	log.info("Resetting build.")
	self.data.api_client.build(self.data.build["id"]).reset.post()

We are only resetting it if has at least one command already. We should remove that restriction 👍🏼

[humitos]

The duplicated "concurrecy limit" notifications deleted and they should not appear again in new builds. As example, the old build linked in this issue doesn't show it anymore: https://beta.readthedocs.org/projects/xclim/builds/23494705/