[discuss] Implement notification: queued for deletion

[mtojek]

Hi Team,

I wanted to start a discussion about how we can implement this, but didn't want to write a full RFC. Long story short, coderd replica has to send a notification when the workspace is scheduled for deletion (3d, 1d, 30min).

Proposal

Let's implement event deduplication similar to Event Sourcing.

1. Every replica periodically checks if the workspace is scheduled for deletion, and its owner should be notified.
2. In the worst case, all replicas take the same decision to insert notification_message.
3. Every notifier follows the same process:
   3.1 Fetch messages using AcquireNotificationMessages.
   3.2 For every message: identify its type and call optional logic to verify if delivered.
   3.3 If already delivered, then skip delivery.
   3.4 Call optional delivery callback.

Verification logic and delivery callback depend on the notification type. For "queued for deletion" it could check against the database column notified_deleting_at. The value is updated by delivery callback.

Let me know your thoughts!

cc @dannykopping @mafredri

[dannykopping]

I think what you're suggesting here is pushing responsibility to inappropriate levels of abstraction.

2. In the worst case, all replicas take the same decision to insert notification_message.

Are you saying this is still possible? If so, I don't understand the rest of the proposal; this is the behaviour we have to avoid.

3.2 For every message: identify its type and call optional logic to verify if delivered.

Why does delivery need to be verified? If there already exists a notification for this specific condition, that is enough to prevent another from being enqueued. The problem is more about how do we determine that a notification exists at all for a given "event".

Verification logic and delivery callback depend on the notification type.

I think this introduces complexity that we probably don't need.

---

If the number of replicas is known at the time of AcquireNotificationMessages, we could do some magic with hash mod to distribute the work, but I'm not sure we do currently? And what if a replica crashes before it can send the notification?

For now, I think we should not implement this notification type.

[mtojek]

Are you saying this is still possible? If so, I don't understand the rest of the proposal; this is the behaviour we have to avoid.

The proposal relies on the fact that it is real and possible, and defers deduplication to notifiers.

If there already exists a notification for this specific condition, that is enough to prevent another from being enqueued.

To achieve this, Coder has to keep a history of all notifications delivered in the last 3 days. This proposal assumes that you can computationally check if the notification has been delivered.

Agree, we could run the validation logic before enqueuing but this would slow down the primary flow. I'd rather run the check asynchronously.

I think this introduces complexity that we probably don't need.

I admit I found this idea here.

we could do some magic with hash mod to distribute the work, but I'm not sure we do currently?

This could be a debate about what is lesser evil. I'd rather do an extra check against the workspace than rely on ID math.

[dannykopping]

Are you saying this is still possible? If so, I don't understand the rest of the proposal; this is the behaviour we have to avoid.

The proposal relies on the fact that it is real and possible, and defers deduplication to notifiers.

To clarify: you're saying that it will not be possible for all notifiers to perform the action simultaneously with your solution in place?

[mtojek]

Yes, I rely on this assumption:

// fetch retrieves messages from the queue by "acquiring a lease" whereby this notifier is the exclusive handler of these
// messages until they are dispatched - or until the lease expires (in exceptional cases).

EDIT:

I'm not claiming that this is a perfect solution, so please challenge it!

[dannykopping]

In my original design, I had the concept of a dedupe_hash which would exist on every notification_message row; it would contain the hash of the other fields, with a unique index to ensure the same exact message could not be enqueued. That's a very hacky way to make this work, and avoids needing to insert n-1 unnecessary rows into the database. I dropped it because we didn't need it at implementation time, and the more I thought about it I realised it was just allowing the database to handle bad design further up the stack.

I still think that the complexity that handling this "queued for deletion" event requires is not worth the value it delivers.

I think we need a more appropriate abstraction such as leader election or distributed locking to ensure that only one replica is responsible for enqueuing notifications in this scenario; pushing the responsibility down to the notifiers is a slippery slope because now they are performing multiple roles.

[mtojek]

In my original design, I had the concept of a dedupe_hash which would exist on every notification_message row; it would contain the hash of the other fields, with a unique index to ensure the same exact message could not be enqueued.

Sooner or later we may need this 👍

I still think that the complexity that handling this "queued for deletion" event requires is not worth the value it delivers.

Fully agree, but it might be beneficial to consider this a playground for the deduplication feature, and use it in the future to implement more serious events.

TBH I'm fine with any option:

1. Postpone it for the future and focus on simpler cases.
2. Switch to dedupe_hash.

I find the leader election to be an overkill for this.

[dannykopping]

Oh I'm not suggesting at all that we need to implement leader election; I'm just saying we need a clean abstraction to implement control over situations like this. Without it, it makes things very difficult / complex, as we're seeing here in this design.

I'm very much in favour of postponing until a customer asks for this, or we encounter other design problems in the same space which allow us to figure out a cleaner design.