if this is required, how were our tests working before?

This is only if you specify a database URL to run tests against. I needed to run a standalone postgres to see the logs and previously it wouldn't run the migrations. I can prob remove this, I'm not sure if it has any greater effects.

Some additional test cases I'd like to see:

1. MultiAgent with multiple coordinators
2. MultiAgent UpdateSelf first, then SubscribeAgent --- verify the agent gets the update.
3. UnsubscribeAgent --- verify that the agent no longer gets updates about the client and vice versa
4. Subscribe to multiple agents

I'll work on implementing these after the rest is solid 👍

this is a memory leak: you add to q.clientSubscriptions but never remove.

You still never delete anything from q.clientSubscriptions

Whoops, I think I got this now.

You still never delete anything from q.clientSubscriptions

the fact that you're checking ok here smells funny and indicates that we're not keeping the data structures in sync.

It should be an invariant in this code that a client subscription in the q.clientSubscriptions map implies that a corresponding mapper exists. They are always added together and always removed together while holding the mutex.

How do you think we should handle a case where the mapper doesn't exist. Panic?

yeah, panic, or at the very least, drop a Critical log

I found a pretty easy way to trigger this invariant. It's possible for (*querier).cleanupConn to race with a call to removeClientSubscription, such that removeClientSubscription is trying to remove a subscription that no longer exists. I'm not too sure how to prevent this, except by ignoring duplicates.

The invariant is that a client subscription exists in q.clientSubscriptions AND in the mappers, or neither. It can never be in one but not the other when the mutex is unlocked.

You're right though, that in this design, it's possible for removeClientSubscription to race with cleanupConn such that you can try to remove a subscription that was already cleaned up. What you've done is fine and follows the invariant --- if the subscription is not in q.clientSubscriptions that means it's already cleaned up from the mappers too, and removeClientSubscription can return early.

you send a subscription with agentID unset on remove. The subscriber handles this case, but then it forwards it to the querier which I don't think handles the case.

It's technically handled by checking the clientSubscriptions map, since there will never be an agent subscription with uuid.Nil. I had an explicit check but just replaced it with the map lookup, since it essentially does the same thing. Might be good to comment.