[FIXED] Avoid stalling read loop on leafnode ErrMinimumVersionRequired

Remove the accept side wait from leaf minimum version rejection
handling and apply the 5s backoff on the soliciting side instead.
That is similar to the same cluster name reconnect delay path.

This keeps the read loop from waiting during rejection and makes the
reconnect behavior explicit on new peers. Note that the previous
accept side 5s delay was not effective: the inbound leaf connection
would be closed earlier by the existing ping timer, the hold period
was not enforced.

Signed-off-by: Daniele Sciascia <daniele@nats.io>

Author: sciascid

server/client.go

@@ -1465,11 +1465,8 @@ func (c *client) readLoop(pre []byte) {
 			if err := c.parse(bufs[i]); err != nil {
 				if err == ErrMinimumVersionRequired {
 					// Special case here, currently only for leaf node connections.
-					// When process the CONNECT protocol, if the minimum version
-					// required was not met, an error was printed and sent back to
-					// the remote, and connection was closed after a certain delay
-					// (to avoid "rapid" reconnection from the remote).
-					// We don't need to do any of the things below, simply return.
+					// processLeafConnect() already sent the rejection and closed
+					// the connection, so there is nothing else to do here.
 					return
 				}
 				if dur := time.Since(c.in.start); dur >= readLoopReportThreshold {

server/errors.go

@@ -207,6 +207,9 @@ var (
 
 	// ErrMinimumVersionRequired is returned when a connection is not at the minimum version required.
 	ErrMinimumVersionRequired = errors.New("minimum version required")
+	// ErrLeafNodeMinVersionRejected is the leafnode protocol error prefix used
+	// when rejecting a remote due to leafnodes.min_version.
+	ErrLeafNodeMinVersionRejected = errors.New("connection rejected since minimum version required is")
 
 	// ErrInvalidMappingDestination is used for all subject mapping destination errors
 	ErrInvalidMappingDestination = errors.New("invalid mapping destination")

server/leafnode.go

@@ -63,9 +63,9 @@ const (
 	// LEAF connection as opposed to a CLIENT.
 	leafNodeWSPath = "/leafnode"
 
-	// This is the time the server will wait, when receiving a CONNECT,
-	// before closing the connection if the required minimum version is not met.
-	leafNodeWaitBeforeClose = 5 * time.Second
+	// When a soliciting leafnode is rejected because it does not meet the
+	// configured minimum version, delay the next reconnect attempt by this long.
+	leafNodeMinVersionReconnectDelay = 5 * time.Second
 )
 
 type leaf struct {
@@ -1885,17 +1885,11 @@ func (c *client) processLeafNodeConnect(s *Server, arg []byte, lang string) erro
 	if mv := s.getOpts().LeafNode.MinVersion; mv != _EMPTY_ {
 		major, minor, update, _ := versionComponents(mv)
 		if !versionAtLeast(proto.Version, major, minor, update) {
-			// We are going to send back an INFO because otherwise recent
-			// versions of the remote server would simply break the connection
-			// after 2 seconds if not receiving it. Instead, we want the
-			// other side to just "stall" until we finish waiting for the holding
-			// period and close the connection below.
+			// Send back an INFO so recent remote servers process the rejection
+			// cleanly, then close immediately. The soliciting side applies the
+			// reconnect delay when it processes the error.
 			s.sendPermsAndAccountInfo(c)
-			c.sendErrAndErr(fmt.Sprintf("connection rejected since minimum version required is %q", mv))
-			select {
-			case <-c.srv.quitCh:
-			case <-time.After(leafNodeWaitBeforeClose):
-			}
+			c.sendErrAndErr(fmt.Sprintf("%s %q", ErrLeafNodeMinVersionRejected, mv))
 			c.closeConnection(MinimumVersionRequired)
 			return ErrMinimumVersionRequired
 		}
@@ -2984,6 +2978,11 @@ func (c *client) leafProcessErr(errStr string) {
 		c.Errorf("Leafnode connection dropped with same cluster name error. Delaying attempt to reconnect for %v", delay)
 		return
 	}
+	if strings.Contains(errStr, ErrLeafNodeMinVersionRejected.Error()) {
+		_, delay := c.setLeafConnectDelayIfSoliciting(leafNodeMinVersionReconnectDelay)
+		c.Errorf("Leafnode connection dropped due to minimum version requirement. Delaying attempt to reconnect for %v", delay)
+		return
+	}
 
 	// We will look for Loop detected error coming from the other side.
 	// If we solicit, set the connect delay.

server/leafnode_test.go

@@ -6041,15 +6041,15 @@ leafnodes:{
 
 type checkLeafMinVersionLogger struct {
 	DummyLogger
-	errCh  chan string
-	connCh chan string
+	errCh  chan time.Time
+	connCh chan time.Time
 }
 
 func (l *checkLeafMinVersionLogger) Errorf(format string, args ...any) {
 	msg := fmt.Sprintf(format, args...)
 	if strings.Contains(msg, "minimum version") {
 		select {
-		case l.errCh <- msg:
+		case l.errCh <- time.Now():
 		default:
 		}
 	}
@@ -6059,7 +6059,7 @@ func (l *checkLeafMinVersionLogger) Noticef(format string, args ...any) {
 	msg := fmt.Sprintf(format, args...)
 	if strings.Contains(msg, "Leafnode connection created") {
 		select {
-		case l.connCh <- msg:
+		case l.connCh <- time.Now():
 		default:
 		}
 	}
@@ -6134,7 +6134,7 @@ func TestLeafNodeMinVersion(t *testing.T) {
 	s, o = RunServerWithConfig(conf)
 	defer s.Shutdown()
 
-	l := &checkLeafMinVersionLogger{errCh: make(chan string, 1), connCh: make(chan string, 1)}
+	l := &checkLeafMinVersionLogger{errCh: make(chan time.Time, 1), connCh: make(chan time.Time, 1)}
 	s.SetLogger(l, false, false)
 
 	rconf = createConfFile(t, []byte(fmt.Sprintf(`
@@ -6156,21 +6156,23 @@ func TestLeafNodeMinVersion(t *testing.T) {
 		t.Fatal("Remote did not try to connect")
 	}
 
+	var rejectAt time.Time
 	select {
-	case <-l.errCh:
+	case rejectAt = <-l.errCh:
 	case <-time.After(time.Second):
 		t.Fatal("Did not get the minimum version required error")
 	}
 
-	// Since we have a very small reconnect interval, if the connection was
-	// closed "right away", then we should have had a reconnect attempt with
-	// another failure. This should not be the case because the server will
-	// wait 5s before closing the connection.
+	// Since we have a very small reconnect interval, the next attempt should be
+	// delayed by the dedicated minimum version reconnect delay on the soliciting
+	// side, not by the normal reconnect interval.
 	select {
-	case <-l.connCh:
-		t.Fatal("Should not have tried to reconnect")
-	case <-time.After(250 * time.Millisecond):
-		// OK
+	case secondAttemptAt := <-l.connCh:
+		if elapsed := secondAttemptAt.Sub(rejectAt); elapsed < leafNodeMinVersionReconnectDelay {
+			t.Fatalf("Expected reconnect attempt after at least %v, got %v", leafNodeMinVersionReconnectDelay, elapsed)
+		}
+	case <-time.After(7 * time.Second):
+		t.Fatal("Did not get the reconnect attempt")
 	}
 }