Merge pull request #525 from slingamn/autobrb.8

hopefully the last round of resume/brb fixes before the release

gencapdefs.py

@@ -113,7 +113,7 @@ CAPDEFS = [
     ),
     CapDef(
         identifier="Resume",
-        name="draft/resume-0.4",
+        name="draft/resume-0.5",
         url="https://github.com/DanielOaks/ircv3-specifications/blob/master+resume/extensions/resume.md",
         standard="proposed IRCv3",
     ),

irc/caps/defs.go

@@ -77,7 +77,7 @@ const (
 	// https://github.com/SaberUK/ircv3-specifications/blob/rename/extensions/rename.md
 	Rename Capability = iota
 
-	// Resume is the proposed IRCv3 capability named "draft/resume-0.4":
+	// Resume is the proposed IRCv3 capability named "draft/resume-0.5":
 	// https://github.com/DanielOaks/ircv3-specifications/blob/master+resume/extensions/resume.md
 	Resume Capability = iota
 
@@ -141,7 +141,7 @@ var (
 		"message-tags",
 		"multi-prefix",
 		"draft/rename",
-		"draft/resume-0.4",
+		"draft/resume-0.5",
 		"sasl",
 		"server-time",
 		"draft/setname",

irc/client.go

@@ -53,6 +53,7 @@ type Client struct {
 	certfp             string
 	channels           ChannelSet
 	ctime              time.Time
+	destroyed          bool
 	exitedSnomaskSent  bool
 	flags              modes.ModeSet
 	hostname           string
@@ -103,6 +104,7 @@ type Session struct {
 
 	idletimer IdleTimer
 	fakelag   Fakelag
+	destroyed uint32
 
 	quitMessage string
 
@@ -146,6 +148,20 @@ func (session *Session) MaxlenRest() int {
 	return int(atomic.LoadUint32(&session.maxlenRest))
 }
 
+// returns whether the session was actively destroyed (for example, by ping
+// timeout or NS GHOST).
+// avoids a race condition between asynchronous idle-timing-out of sessions,
+// and a condition that allows implicit BRB on connection errors (since
+// destroy()'s socket.Close() appears to socket.Read() as a connection error)
+func (session *Session) Destroyed() bool {
+	return atomic.LoadUint32(&session.destroyed) == 1
+}
+
+// sets the timed-out flag
+func (session *Session) SetDestroyed() {
+	atomic.StoreUint32(&session.destroyed, 1)
+}
+
 // WhoWas is the subset of client details needed to answer a WHOWAS query
 type WhoWas struct {
 	nick           string
@@ -373,7 +389,7 @@ func (client *Client) run(session *Session) {
 		client.nickTimer.Initialize(client)
 	}
 
-	firstLine := true
+	firstLine := !isReattach
 
 	for {
 		maxlenRest := session.MaxlenRest()
@@ -386,8 +402,10 @@ func (client *Client) run(session *Session) {
 			}
 			client.Quit(quitMessage, session)
 			// since the client did not actually send us a QUIT,
-			// give them a chance to resume or reattach if applicable:
+			// give them a chance to resume if applicable:
-			client.brbTimer.Enable()
+			if !session.Destroyed() {
+				client.brbTimer.Enable()
+			}
 			break
 		}
 
@@ -396,7 +414,7 @@ func (client *Client) run(session *Session) {
 		}
 
 		// special-cased handling of PROXY protocol, see `handleProxyCommand` for details:
-		if !isReattach && firstLine {
+		if firstLine {
 			firstLine = false
 			if strings.HasPrefix(line, "PROXY") {
 				err = handleProxyCommand(client.server, client, session, line)
@@ -562,14 +580,14 @@ func (session *Session) playResume() {
 
 	timestamp := session.resumeDetails.Timestamp
 	gap := lastDiscarded.Sub(timestamp)
-	session.resumeDetails.HistoryIncomplete = gap > 0
+	session.resumeDetails.HistoryIncomplete = gap > 0 || timestamp.IsZero()
 	gapSeconds := int(gap.Seconds()) + 1 // round up to avoid confusion
 
 	details := client.Details()
 	oldNickmask := details.nickMask
 	client.SetRawHostname(session.rawHostname)
 	hostname := client.Hostname() // may be a vhost
-	timestampString := session.resumeDetails.Timestamp.Format(IRCv3TimestampFormat)
+	timestampString := timestamp.Format(IRCv3TimestampFormat)
 
 	// send quit/resume messages to friends
 	for friend := range friends {
@@ -578,23 +596,29 @@ func (session *Session) playResume() {
 		}
 		for _, fSession := range friend.Sessions() {
 			if fSession.capabilities.Has(caps.Resume) {
-				if timestamp.IsZero() {
+				if !session.resumeDetails.HistoryIncomplete {
-					fSession.Send(nil, oldNickmask, "RESUMED", hostname)
+					fSession.Send(nil, oldNickmask, "RESUMED", hostname, "ok")
-				} else {
+				} else if session.resumeDetails.HistoryIncomplete && !timestamp.IsZero() {
 					fSession.Send(nil, oldNickmask, "RESUMED", hostname, timestampString)
+				} else {
+					fSession.Send(nil, oldNickmask, "RESUMED", hostname)
 				}
 			} else {
-				if session.resumeDetails.HistoryIncomplete {
+				if !session.resumeDetails.HistoryIncomplete {
-					fSession.Send(nil, oldNickmask, "QUIT", fmt.Sprintf(friend.t("Client reconnected (up to %d seconds of history lost)"), gapSeconds))
-				} else {
 					fSession.Send(nil, oldNickmask, "QUIT", fmt.Sprintf(friend.t("Client reconnected")))
+				} else if session.resumeDetails.HistoryIncomplete && !timestamp.IsZero() {
+					fSession.Send(nil, oldNickmask, "QUIT", fmt.Sprintf(friend.t("Client reconnected (up to %d seconds of message history lost)"), gapSeconds))
+				} else {
+					fSession.Send(nil, oldNickmask, "QUIT", fmt.Sprintf(friend.t("Client reconnected (message history may have been lost)")))
 				}
 			}
 		}
 	}
 
-	if session.resumeDetails.HistoryIncomplete {
+	if session.resumeDetails.HistoryIncomplete && !timestamp.IsZero() {
 		session.Send(nil, client.server.name, "WARN", "RESUME", "HISTORY_LOST", fmt.Sprintf(client.t("Resume may have lost up to %d seconds of history"), gapSeconds))
+	} else {
+		session.Send(nil, client.server.name, "WARN", "RESUME", "HISTORY_LOST", client.t("Resume may have lost some message history"))
 	}
 
 	session.Send(nil, client.server.name, "RESUME", "SUCCESS", details.nick)
@@ -942,10 +966,10 @@ func (client *Client) Quit(message string, session *Session) {
 func (client *Client) destroy(session *Session) {
 	var sessionsToDestroy []*Session
 
-	// allow destroy() to execute at most once
 	client.stateMutex.Lock()
 	details := client.detailsNoMutex()
 	brbState := client.brbTimer.state
+	brbAt := client.brbTimer.brbAt
 	wasReattach := session != nil && session.client != client
 	sessionRemoved := false
 	var remainingSessions int
@@ -959,6 +983,16 @@ func (client *Client) destroy(session *Session) {
 			sessionsToDestroy = []*Session{session}
 		}
 	}
+
+	// should we destroy the whole client this time?
+	// BRB is not respected if this is a destroy of the whole client (i.e., session == nil)
+	brbEligible := session != nil && (brbState == BrbEnabled || brbState == BrbSticky)
+	shouldDestroy := !client.destroyed && remainingSessions == 0 && !brbEligible
+	if shouldDestroy {
+		// if it's our job to destroy it, don't let anyone else try
+		client.destroyed = true
+	}
+	exitedSnomaskSent := client.exitedSnomaskSent
 	client.stateMutex.Unlock()
 
 	// destroy all applicable sessions:
@@ -972,6 +1006,7 @@ func (client *Client) destroy(session *Session) {
 		// send quit/error message to client if they haven't been sent already
 		client.Quit("", session)
 		quitMessage = session.quitMessage
+		session.SetDestroyed()
 		session.socket.Close()
 
 		// remove from connection limits
@@ -991,7 +1026,7 @@ func (client *Client) destroy(session *Session) {
 	}
 
 	// do not destroy the client if it has either remaining sessions, or is BRB'ed
-	if remainingSessions != 0 || brbState == BrbEnabled || brbState == BrbSticky {
+	if !shouldDestroy {
 		return
 	}
 
@@ -1056,14 +1091,23 @@ func (client *Client) destroy(session *Session) {
 		client.server.stats.ChangeOperators(-1)
 	}
 
-	for friend := range friends {
+	// this happens under failure to return from BRB
-		if quitMessage == "" {
+	if quitMessage == "" {
-			quitMessage = "Exited"
+		if !brbAt.IsZero() {
+			awayMessage := client.AwayMessage()
+			if awayMessage != "" {
+				quitMessage = fmt.Sprintf("%s [%s ago]", awayMessage, time.Since(brbAt).Truncate(time.Second).String())
+			}
 		}
+	}
+	if quitMessage == "" {
+		quitMessage = "Exited"
+	}
+	for friend := range friends {
 		friend.sendFromClientInternal(false, splitQuitMessage.Time, splitQuitMessage.Msgid, details.nickMask, details.accountName, nil, "QUIT", quitMessage)
 	}
 
-	if !client.exitedSnomaskSent && registered {
+	if !exitedSnomaskSent && registered {
 		client.server.snomasks.Send(sno.LocalQuits, fmt.Sprintf(ircfmt.Unescape("%s$r exited the network"), details.nick))
 	}
 }

irc/getters.go

@@ -97,14 +97,8 @@ func (client *Client) AddSession(session *Session) (success bool) {
 	defer client.stateMutex.Unlock()
 
 	// client may be dying and ineligible to receive another session
-	switch client.brbTimer.state {
+	if client.destroyed {
-	case BrbDisabled:
-		if len(client.sessions) == 0 {
-			return false
-		}
-	case BrbDead:
 		return false
-		// default: BrbEnabled or BrbSticky, proceed
 	}
 	// success, attach the new session to the client
 	session.client = client
@@ -187,6 +181,12 @@ func (client *Client) SetAway(away bool, awayMessage string) (changed bool) {
 	return
 }
 
+func (client *Client) SetExitedSnomaskSent() {
+	client.stateMutex.Lock()
+	client.exitedSnomaskSent = true
+	client.stateMutex.Unlock()
+}
+
 // uniqueIdentifiers returns the strings for which the server enforces per-client
 // uniqueness/ownership; no two clients can have colliding casefolded nicks or
 // skeletons.

irc/handlers.go

@@ -1052,7 +1052,7 @@ func dlineHandler(server *Server, client *Client, msg ircmsg.IrcMessage, rb *Res
 		}
 
 		for _, mcl := range clientsToKill {
-			mcl.exitedSnomaskSent = true
+			mcl.SetExitedSnomaskSent()
 			mcl.Quit(fmt.Sprintf(mcl.t("You have been banned from this server (%s)"), reason), nil)
 			if mcl == client {
 				killClient = true
@@ -1362,7 +1362,7 @@ func killHandler(server *Server, client *Client, msg ircmsg.IrcMessage, rb *Resp
 	quitMsg := fmt.Sprintf("Killed (%s (%s))", client.nick, comment)
 
 	server.snomasks.Send(sno.LocalKills, fmt.Sprintf(ircfmt.Unescape("%s$r was killed by %s $c[grey][$r%s$c[grey]]"), target.nick, client.nick, comment))
-	target.exitedSnomaskSent = true
+	target.SetExitedSnomaskSent()
 
 	target.Quit(quitMsg, nil)
 	target.destroy(nil)
@@ -1489,7 +1489,7 @@ func klineHandler(server *Server, client *Client, msg ircmsg.IrcMessage, rb *Res
 		}
 
 		for _, mcl := range clientsToKill {
-			mcl.exitedSnomaskSent = true
+			mcl.SetExitedSnomaskSent()
 			mcl.Quit(fmt.Sprintf(mcl.t("You have been banned from this server (%s)"), reason), nil)
 			if mcl == client {
 				killClient = true

irc/idletimer.go

@@ -334,6 +334,7 @@ type BrbTimer struct {
 	client *Client
 
 	state    BrbState
+	brbAt    time.Time
 	duration time.Duration
 	timer    *time.Timer
 }
@@ -344,9 +345,7 @@ func (bt *BrbTimer) Initialize(client *Client) {
 
 // attempts to enable BRB for a client, returns whether it succeeded
 func (bt *BrbTimer) Enable() (success bool, duration time.Duration) {
-	// BRB only makes sense if a new connection can attach to the session;
+	if !bt.client.Registered() || bt.client.ResumeID() == "" {
-	// this can happen either via RESUME or via bouncer reattach
-	if bt.client.Account() == "" && bt.client.ResumeID() == "" {
 		return
 	}
 
@@ -361,6 +360,11 @@ func (bt *BrbTimer) Enable() (success bool, duration time.Duration) {
 		bt.state = BrbEnabled
 		bt.duration = duration
 		bt.resetTimeout()
+		// only track the earliest BRB, if multiple sessions are BRB'ing at once
+		// TODO(#524) this is inaccurate in case of an auto-BRB
+		if bt.brbAt.IsZero() {
+			bt.brbAt = time.Now().UTC()
+		}
 		success = true
 	case BrbSticky:
 		success = true
@@ -373,14 +377,17 @@ func (bt *BrbTimer) Enable() (success bool, duration time.Duration) {
 
 // turns off BRB for a client and stops the timer; used on resume and during
 // client teardown
-func (bt *BrbTimer) Disable() {
+func (bt *BrbTimer) Disable() (brbAt time.Time) {
 	bt.client.stateMutex.Lock()
 	defer bt.client.stateMutex.Unlock()
 
 	if bt.state == BrbEnabled {
 		bt.state = BrbDisabled
+		brbAt = bt.brbAt
+		bt.brbAt = time.Time{}
 	}
 	bt.resetTimeout()
+	return
 }
 
 func (bt *BrbTimer) resetTimeout() {