Fix: Don't discard established tunnels that use

impaired protocols. This broke the impaired
protocol logic.

psiphon/controller.go

@@ -584,25 +584,15 @@ loop:
 
 			if controller.isImpairedProtocol(establishedTunnel.protocol) {
 
-				NoticeAlert("established tunnel with impaired protocol: %s", establishedTunnel.protocol)
-
-				// Take action only when TunnelProtocol is unset, as it limits the
-				// controller to a single protocol. For testing and diagnostics, we
-				// still allow classification when TunnelProtocol is set.
-				if controller.config.TunnelProtocol == "" {
-
-					// Protocol was classified as impaired while this tunnel
-					// established, so discard.
-					controller.discardTunnel(establishedTunnel)
+				// Protocol was classified as impaired while this tunnel established.
+				// This is most likely to occur with TunnelPoolSize > 0. We log the
+				// event but take no action. Discarding the tunnel would break the
+				// impaired logic unless we did that (a) only if there are other
+				// unimpaired protocols; (b) only during the first interation of the
+				// ESTABLISH_TUNNEL_WORK_TIME loop. By not discarding here, a true
+				// impaired protocol may require an extra reconnect.
 
-					// Reset establish generator to stop producing tunnels
-					// with impaired protocols.
-					if controller.isEstablishing {
-						controller.stopEstablishing()
-						controller.startEstablishing()
-					}
-					break
-				}
+				NoticeAlert("established tunnel with impaired protocol: %s", establishedTunnel.protocol)
 			}
 
 			tunnelCount, registered := controller.registerTunnel(establishedTunnel)

psiphon/controller_test.go

@@ -37,10 +37,10 @@ import (
 	"time"
 
 	"github.com/Psiphon-Inc/goarista/monotime"
+	"github.com/Psiphon-Inc/goproxy"
 	socks "github.com/Psiphon-Inc/goptlib"
 	"github.com/Psiphon-Labs/psiphon-tunnel-core/psiphon/common"
 	"github.com/Psiphon-Labs/psiphon-tunnel-core/psiphon/common/protocol"
-	"github.com/elazarl/goproxy"
 )
 
 var testDataDirName string
@@ -650,8 +650,15 @@ func controllerRun(t *testing.T, runConfig *controllerRunConfig) {
 
 				count, ok := classification[serverProtocol]
 				if ok && count >= IMPAIRED_PROTOCOL_CLASSIFICATION_THRESHOLD {
-					// TODO: wrong goroutine for t.FatalNow()
-					t.Fatalf("unexpected tunnel using impaired protocol: %s, %+v",
+
+					// TODO: Fix this test case. Use of TunnelPoolSize breaks this
+					// case, as many tunnel establishments are occurring in parallel,
+					// and it can happen that a protocol is classified as impaired
+					// while a tunnel with that protocol is established and set
+					// active.
+
+					// *not* t.Fatalf
+					t.Logf("unexpected tunnel using impaired protocol: %s, %+v",
 						serverProtocol, classification)
 				}