Previously, uc.network.Network.Nick wasn't successfully updated on
downstream NICK. This would cause soju to immediately switch back to the
old nick when the upstream supported MONITOR, so long as the network had
a nick configured as of initialization.
---
There's also a similar issue with SANICK, since upstream NICK doesn't
update the database either, but I'm not sure what the desired behavior
is there. Thoughts?
 downstream.go | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/downstream.go b/downstream.go
index f9d1d38..3d33298 100644
--- a/downstream.go
+++ b/downstream.go
@@ -1821,9 +1821,10 @@ func (dc *downstreamConn) handleMessageRegistered(ctx context.Context, msg *irc.
 
 		var err error
 		if dc.network != nil {
-			record := dc.network.Network
-			record.Nick = nick
-			err = dc.srv.db.StoreNetwork(ctx, dc.user.ID, &record)
+			// Need to make sure that network.Network.Nick is updated
+			dc.network.Network.Nick = nick
+			err = dc.srv.db.StoreNetwork(ctx, dc.user.ID,
+				&dc.network.Network)
 		} else {
 			record := dc.user.User
 			record.Nick = nick
-- 
2.37.1

Ah, good catch!

One small nit: the code was written this way to properly handle DB errors.
IOW, we shouldn't update dc.network.Network.Nick if the DB fails the update.

Maybe instead  of this patch we can keep the current logic and set
dc.network.Network.Nick if err == nill?

On Mon Aug 22, 2022 at 9:03 AM UTC, Simon Ser wrote:
> Maybe instead  of this patch we can keep the current logic and set
> dc.network.Network.Nick if err == nill?

+1, but before I send v2, what're your thoughts on the SANICK issue I
brought up in the timely commentary? Nothing is updated on receiving
NICKs from upstream (at least, as far as I can tell - I haven't dug too
much into that part of the code yet), so we immediately undo them. This
doesn't seem great, but updating the user's preferred nick based on what
the server tells us doesn't sound like the right thing to do either.

On Monday, August 22nd, 2022 at 13:38, Ember Sawady <ecs@d2evs.net> wrote:

> On Mon Aug 22, 2022 at 9:03 AM UTC, Simon Ser wrote:
> 
> > Maybe instead of this patch we can keep the current logic and set
> > dc.network.Network.Nick if err == nill?
> 
> +1, but before I send v2, what're your thoughts on the SANICK issue I
> brought up in the timely commentary? Nothing is updated on receiving
> NICKs from upstream (at least, as far as I can tell - I haven't dug too
> much into that part of the code yet), so we immediately undo them. This
> doesn't seem great, but updating the user's preferred nick based on what
> the server tells us doesn't sound like the right thing to do either.

Hmm. We never update our DB when upstream changes something: our DB
represents the user's desired state. So yeah, I don't think it's
correct to do that here.

That said, we should avoid mis-behaving when the server changes our
nick. Maybe our logic to regain our desired nick should only be triggered
once when connecting? Maybe we can set a flag in upstreamConn when falling
back to a nick with underscore suffixes, and only try to regain our desired
nick if that flag is set?