Set notification status to done for dependent builds.

[kquick]

When a build completes with error, the dependent builds are updated
with errors as well, and notifications are sent for all these builds,
but the DB wasn't setting the notification completion indication for
the dependent builds. One effect of this is that on hydra restart,
notifications would again be sent for all dependent build failures,
causing duplicate notifications, many of which were often very old.

[grahamc]

I'm not really able to determine if this is a proper fix, or more of a hack?

[kquick]

Thank you for taking a look at this PR, @grahamc. This is intended to be a proper fix.

The code following this block will send the notifications for this build and all of its dependents:

hydra/src/hydra-queue-runner/builder.cc

Lines 466 to 471 in 1e804d1

	/* Send notification about this build and its dependents. */
	{
	auto notificationSenderQueue_(notificationSenderQueue.lock());
	notificationSenderQueue_->push(NotificationItem{NotificationItem::Type::BuildFinished, buildId, dependentIDs});
	}
	notificationSenderWakeup.notify_one();

This modification ensures that the notificationPendingSince is set to zero (the zero value indicates that the notification has been sent). If the value is non-zero, then the code below will see that there is a pending notification and repeat the notification attempts done in 466-471 above, causing duplicate notifications:

hydra/src/hydra-queue-runner/hydra-queue-runner.cc

Lines 850 to 861 in 1e804d1

	/* Enqueue notification items for builds that were finished
	previously, but for which we didn't manage to send
	notifications. */
	{
	auto conn(dbPool.get());
	pqxx::work txn(*conn);
	auto res = txn.parameterized("select id from Builds where notificationPendingSince > 0").exec();
	for (auto const & row : res) {
	auto id = row["id"].as<BuildID>();
	enqueueNotificationItem({NotificationItem::Type::BuildFinished, id});
	}
	}

This fix is based on our local running hydra and the complaints of users, which this fix resolved. The alternative would be to remove lines 466-471 in builder.cc and let the hydra-queue-runner.cc handle the notifications, but it does seem to communicate the intent more directly to instigate those notifications in 466-471.

[grahamc]

I have a new appreciation for this PR after reviewing the notification work. I'm not sure this PR is the right thing: it seems to me that the right behavior is:

1. notify that the top level build finished, with the dependent IDs
2. also send a unique notification per build ID

then let hydra-notify handle the per-build marking as notified. If this isn't the right behavior, then maybe hydra-notify should run the handlers per ID in the payload, and not just pass the extra IDs as an additional argument.

[kquick]

Independently addressed in 4417f9f

[grahamc]

Interesting. Okay :) 🤷