I kept the terminology aligned with the current workspace agent app status (Working,Idle). In PR:https://github.com/coder/coder/pull/19773/files#diff-1894d4f2ccb1125739e504fc6d8d3a7daeec9dd829a81c342cf28f3e92efbf58R2-R7 I see the task status types differ. Is that finalized? If yes, should I update this PR to adopt the new task status terms?

With the current data model, this is the correct way to determine if a workspace agent app is an AI task, right?

For now, yes. This will need to be refactored later.

For now, yes. This will need to be refactored later.

Before we enqueue a notification, we should ensure that the new task status is different to the previous. An agent could insert multiple app statuses with the same state but different messages e.g.

• "working / doing thing A"
• "working / doing thing B"

Ok, that is good to know. I was not aware it was possible for a task to transition to the same status. Addressed in06ed8c7

How does this work if the task quickly jumps between working/idle/working? Is there a chance we miss the second working and never emit it? If yes, I think that would lead to bad UX as the notifications are untrustworthy.

If a task were to go through the state transitionIdle -> Working -> Idle -> Working -> Idle all within the space of a minute, you'd only get the initial-> Working and-> Idle notifications.

Thanks for confirming, that's what I suspected. It gets a bit funky when we're bordering on the minute mark.

14:39:00 Working
14:39:29 Idle
14:39:59 Working
14:40:30 Idle

We get Working -> Idle -> Idle (alt. Idle -> Working -> Working).

Yes, that’s a good point. There are still scenarios where the system could skip some notification types, resulting in the same type being sent back-to-back. The minute-level dedupe was intended as a safeguard to avoid spamming users. Based on the problem definition "By definition tasks may take quite a long time to be completed. We do not want the users to stare at the screen waiting because that invalidates the whole purpose." the assumption was that these transitions wouldn’t happen very quickly.

Instead of truncating to the minute, should we use the exact timestamp? That would avoid missing intermediate states, but it does increase the risk of too many notifications. Open to suggestions 😊

Unfortunately I think there is no perfect solution here. Wecould still truncate to the second, but like before, any truncation runs the risk of missing a notification. I'd be satisfied with truncating it down to the second.@mafredri thoughts?

I'd rather risk missing some notifications than spamming and potentially overloading coderd or whatever notification backed is in use.

Second or even 5-10 second truncate would work. 👍🏻 Especially if we implement the safeguard that Cian mentioned, i.e. previous status must have been different from current status, then we minimize the risk of spam.

Added a 10-second truncate to the dedupe key and a check to send notifications only on new transitions.
Addressed in06ed8c7

Obligatory reminder to check migration number before merge!

Would it make sense to instead add anORDER BY clause toGetWorkspaceAppStatusesByAppIDs?

We could addORDER BY toGetWorkspaceAppStatusesByAppIDs and rename it to reflect the ordering (e.g.,GetLatestWorkspaceAppStatusesByAppIDs). For this PR I only need one app, newest-first, so the dedicated query seemed to keep things clearer.

I realize the other functions around here do the same but I think we were wanting to stop introducing more uses ofResourceSystem

AFAIK, there isn’t a dedicated resource for workspace apps, so operations on them currently authorize againstrbac.ResourceSystem. I’m not sure what the best alternative is, skipping the authorization check would make this data accessible to anyone, which we don’t want.

This is unfortunately a pre-existing issue from the introduction of app statuses. It may be worth addressing in a follow-up.