Code review fixes

psiphon/common/inproxy/quality.go

@@ -436,7 +436,7 @@ func (r *ProxyQualityReporter) ReportQuality(
 	// reports are simply dropped. There is no back pressure to slow down the
 	// rate of quality tunnels, since the overall goal is to establish
 	// quality tunnels.
-	if r.reportQueue.Len() > proxyQualityReporterMaxQueueEntries {
+	if r.reportQueue.Len() >= proxyQualityReporterMaxQueueEntries {
 		r.logger.WithTrace().Warning("proxyQualityReporterMaxQueueEntries exceeded")
 		return
 	}
@@ -511,22 +511,18 @@ func (r *ProxyQualityReporter) requestScheduler(ctx context.Context) {
 		//   counts for the same proxy ID and client ASN are TTL extensions
 		//   in the ProxyQualityState.
 
-		for r.hasMoreRequests() {
-
+		for {
 			requestCounts := r.prepareNextRequest()
 
+			if len(requestCounts) == 0 {
+				break
+			}
+
 			r.sendToBrokers(ctx, brokerClients, requestCounts)
 		}
 	}
 }
 
-func (r *ProxyQualityReporter) hasMoreRequests() bool {
-	r.mutex.Lock()
-	defer r.mutex.Unlock()
-
-	return r.reportQueue.Len() > 0
-}
-
 func (r *ProxyQualityReporter) prepareNextRequest() ProxyQualityRequestCounts {
 	r.mutex.Lock()
 	defer r.mutex.Unlock()
@@ -534,6 +530,10 @@ func (r *ProxyQualityReporter) prepareNextRequest() ProxyQualityRequestCounts {
 	// prepareNextRequest should not hold the mutex for a long period, as this
 	// blocks ReportQuality, which in turn could block tunnel I/O operations.
 
+	if r.reportQueue.Len() == 0 {
+		return nil
+	}
+
 	counts := make(ProxyQualityRequestCounts)
 
 	queueEntry := r.reportQueue.Front()
@@ -704,10 +704,14 @@ func (r *ProxyQualityReporter) sendToBrokers(
 						common.LogFields{"brokerID": brokerClient.publicKey, "error": err.Error()},
 					).Warning("sendBrokerRequest failed")
 					if i < retries {
+						// Try again.
 						continue
 					}
+					// No more retries, and don't retain the brokerClient.
 					return
 				}
+				// Exit the retry loop, and retain the successful brokerClient.
+				break
 			}
 
 			// Retain the successful brokerClient.

psiphon/common/inproxy/quality_test.go

@@ -161,12 +161,12 @@ func runTestProxyQualityReporter() error {
 
 	for count := 0; count < expectedRequestCount; count++ {
 
-		if !r.hasMoreRequests() {
-			return errors.TraceNew("unexpected hasMoreRequests")
-		}
-
 		requestCounts := r.prepareNextRequest()
 
+		if len(requestCounts) == 0 {
+			return errors.TraceNew("unexpected requestCounts")
+		}
+
 		for i := count * 10; i < count*10+10; i++ {
 			counts, ok := requestCounts[proxyKeys[i]]
 			if !ok {
@@ -186,8 +186,8 @@ func runTestProxyQualityReporter() error {
 
 	}
 
-	if r.hasMoreRequests() {
-		return errors.TraceNew("unexpected hasMoreRequests")
+	if len(r.prepareNextRequest()) != 0 {
+		return errors.TraceNew("unexpected prepareNextRequest")
 	}
 
 	return nil

psiphon/common/parameters/parameters.go

@@ -506,6 +506,7 @@ const (
 	// Retired parameters
 
 	ReplayRandomizedTLSProfile = "ReplayRandomizedTLSProfile"
+	InproxyAllBrokerPublicKeys = "InproxyAllBrokerPublicKeys"
 )
 
 const (
@@ -968,6 +969,7 @@ var defaultParameters = map[string]struct {
 	InproxyAllowClient:                                 {value: false, flags: serverSideOnly},
 	InproxyAllowDomainFrontedDestinations:              {value: false, flags: serverSideOnly},
 	InproxyTunnelProtocolSelectionProbability:          {value: 0.5, minimum: 0.0},
+	InproxyAllBrokerPublicKeys:                         {value: []string{}, flags: serverSideOnly},
 	InproxyAllBrokerSpecs:                              {value: InproxyBrokerSpecsValue{}, flags: serverSideOnly},
 	InproxyBrokerSpecs:                                 {value: InproxyBrokerSpecsValue{}},
 	InproxyPersonalPairingBrokerSpecs:                  {value: InproxyBrokerSpecsValue{}},
@@ -1325,6 +1327,13 @@ func (p *Parameters) Set(
 	// must appear in InproxyAllCommonCompartmentIDs. This check is
 	// server-side only as the "All" parameters are serverSideOnly.
 
+	// Note that similar validation for InproxyAllBrokerPublicKeys has been
+	// retired, although InproxyAllBrokerPublicKeys is still present in
+	// defaultParameters for backwards compatibility during the transition to
+	// InproxyAllBrokerSpecs.
+	//
+	// TODO: fully retire InproxyAllBrokerPublicKeys.
+
 	checkInproxyLists := !skipOnError && serverSide
 
 	inproxyAllBrokerSpecsValue, err := getAppliedValue(