Fix bugs and comments

psiphon/controller.go

@@ -569,9 +569,6 @@ loop:
 
 		egressRegion := controller.config.EgressRegion
 		constraints := request.constraints
-		// When CountServerEntriesWithConstraints is called only
-		// limitTunnelProtocolState is fixed; excludeIntensive is transitory.
-		excludeIntensive := false
 
 		var response serverEntriesReportResponse
 
@@ -579,23 +576,36 @@ loop:
 
 		callback := func(serverEntry *protocol.ServerEntry) bool {
 
+			// In establishment, excludeIntensive depends on what set of protocols are
+			// already being dialed. For these reports, don't exclude intensive
+			// protocols as any intensive candidate can always be an available
+			// candidate at some point.
+			excludeIntensive := false
+
 			isInitialCandidate := constraints.isInitialCandidate(excludeIntensive, serverEntry)
 			isCandidate := constraints.isCandidate(excludeIntensive, serverEntry)
 
 			if isInitialCandidate {
-				if egressRegion == "" || serverEntry.Region == egressRegion {
+				response.initialCandidatesAnyEgressRegion += 1
+			}
+
+			if egressRegion == "" || serverEntry.Region == egressRegion {
+				if isInitialCandidate {
 					response.initialCandidates += 1
 				}
-				response.initialCandidatesAnyEgressRegion += 1
+				if isCandidate {
+					response.candidates += 1
+				}
 			}
 
-			if isCandidate {
-				response.candidates += 1
+			isAvailable := isCandidate
+			if constraints.hasInitialProtocols() {
+				// Available egress regions is subject to an initial limit constraint, if
+				// present: see AvailableEgressRegions comment in launchEstablishing.
+				isAvailable = isInitialCandidate
 			}
 
-			// Available egress regions is subject to an initial limit constraint, if
-			// present: see AvailableEgressRegions comment in launchEstablishing.
-			if (constraints.hasInitialProtocols() && isInitialCandidate) || isCandidate {
+			if isAvailable {
 				// Ignore server entries with no region field.
 				if serverEntry.Region != "" {
 					regions[serverEntry.Region] = true
@@ -1717,7 +1727,7 @@ func (controller *Controller) establishCandidateGenerator() {
 
 	applyServerAffinity, iterator, err := NewServerEntryIterator(controller.config)
 	if err != nil {
-		NoticeError("failed to iterate over candidates: %s", errors.Trace(err))
+		NoticeError("failed to iterate over candidates: %v", errors.Trace(err))
 		controller.SignalComponentFailure()
 		return
 	}

psiphon/dataStore.go

@@ -1273,6 +1273,25 @@ func deleteServerEntryHelper(
 // ScanServerEntries where possible; and use the canel option to interrupt
 // scans that are no longer required.
 func ScanServerEntries(callback func(*protocol.ServerEntry) bool) error {
+
+	// TODO: this operation can be sped up (by a factor of ~2x, in one test
+	// scenario) by using a faster JSON implementation
+	// (https://github.com/json-iterator/go) and increasing
+	// datastoreServerEntryFetchGCThreshold.
+	//
+	// json-iterator increases the binary code size significantly, which affects
+	// memory limit accounting on some platforms, so it's not clear we can use it
+	// universally. Similarly, tuning datastoreServerEntryFetchGCThreshold has a
+	// memory limit tradeoff.
+	//
+	// Since ScanServerEntries is now called asynchronously and doesn't block
+	// establishment at all, we can tolerate its slower performance. Other
+	// bulk-JSON operations such as [Streaming]StoreServerEntries also benefit
+	// from using a faster JSON implementation, but the relative performance
+	// increase is far smaller as import times are dominated by data store write
+	// transaction overhead. Other operations such as ServerEntryIterator
+	// amortize the cost of JSON unmarshalling over many other operations.
+
 	err := datastoreView(func(tx *datastoreTx) error {
 
 		bucket := tx.bucket(datastoreServerEntriesBucket)