Resumable fetch remote server list automated tests

* Added test case for interrupted remote server list fetch
* Speed up tests with custom retry periods
* Fetch remote server list bug fixes
* Config param for explicit location of server list download file

psiphon/config.go

@@ -47,12 +47,12 @@ const (
 	TUNNEL_SSH_KEEP_ALIVE_PROBE_INACTIVE_PERIOD    = 10 * time.Second
 	ESTABLISH_TUNNEL_TIMEOUT_SECONDS               = 300
 	ESTABLISH_TUNNEL_WORK_TIME                     = 60 * time.Second
-	ESTABLISH_TUNNEL_PAUSE_PERIOD                  = 5 * time.Second
+	ESTABLISH_TUNNEL_PAUSE_PERIOD_SECONDS          = 5
 	ESTABLISH_TUNNEL_SERVER_AFFINITY_GRACE_PERIOD  = 1 * time.Second
 	HTTP_PROXY_ORIGIN_SERVER_TIMEOUT_SECONDS       = 15
 	HTTP_PROXY_MAX_IDLE_CONNECTIONS_PER_HOST       = 50
 	FETCH_REMOTE_SERVER_LIST_TIMEOUT_SECONDS       = 30
-	FETCH_REMOTE_SERVER_LIST_RETRY_PERIOD          = 30 * time.Second
+	FETCH_REMOTE_SERVER_LIST_RETRY_PERIOD_SECONDS  = 30
 	FETCH_REMOTE_SERVER_LIST_STALE_PERIOD          = 6 * time.Hour
 	PSIPHON_API_CLIENT_SESSION_ID_LENGTH           = 16
 	PSIPHON_API_SERVER_TIMEOUT_SECONDS             = 20
@@ -67,7 +67,7 @@ const (
 	PSIPHON_API_TUNNEL_STATS_MAX_COUNT             = 1000
 	FETCH_ROUTES_TIMEOUT_SECONDS                   = 60
 	DOWNLOAD_UPGRADE_TIMEOUT                       = 15 * time.Minute
-	DOWNLOAD_UPGRADE_RETRY_PERIOD                  = 5 * time.Second
+	DOWNLOAD_UPGRADE_RETRY_PERIOD_SECONDS          = 30
 	DOWNLOAD_UPGRADE_STALE_PERIOD                  = 6 * time.Hour
 	IMPAIRED_PROTOCOL_CLASSIFICATION_DURATION      = 2 * time.Minute
 	IMPAIRED_PROTOCOL_CLASSIFICATION_THRESHOLD     = 3
@@ -95,11 +95,6 @@ type Config struct {
 	// continue running.
 	DataStoreDirectory string
 
-	// DataStoreTempDirectory is the directory in which to store temporary
-	// work files associated with the persistent database.
-	// This parameter is deprecated and may be removed.
-	DataStoreTempDirectory string
-
 	// PropagationChannelId is a string identifier which indicates how the
 	// Psiphon client was distributed. This parameter is required.
 	// This value is supplied by and depends on the Psiphon Network, and is
@@ -120,6 +115,14 @@ type Config struct {
 	// typically embedded in the client binary.
 	RemoteServerListUrl string
 
+	// RemoteServerListDownloadFilename specifies a target filename for
+	// storing the remote server list download. Data is stored in co-located
+	// files (RemoteServerListDownloadFilename.part*) to allow for resumable
+	// downloading. If not specified, the default is to use the
+	// remote object name as the filename, stored in the current working
+	// directory.
+	RemoteServerListDownloadFilename string
+
 	// RemoteServerListSignaturePublicKey specifies a public key that's
 	// used to authenticate the remote server list payload.
 	// This value is supplied by and depends on the Psiphon Network, and is
@@ -264,6 +267,8 @@ type Config struct {
 
 	// UpgradeDownloadFilename is the local target filename for an upgrade download.
 	// This parameter is required when UpgradeDownloadUrl is specified.
+	// Data is stored in co-located files (UpgradeDownloadFilename.part*) to allow
+	// for resumable downloading.
 	UpgradeDownloadFilename string
 
 	// EmitBytesTransferred indicates whether to emit periodic notices showing
@@ -312,49 +317,65 @@ type Config struct {
 
 	// TunnelConnectTimeoutSeconds specifies a single tunnel connection sequence timeout.
 	// Zero value means that connection process will not time out.
-	// If omitted default value is TUNNEL_CONNECT_TIMEOUT_SECONDS.
+	// If omitted, the default value is TUNNEL_CONNECT_TIMEOUT_SECONDS.
 	TunnelConnectTimeoutSeconds *int
 
 	// TunnelPortForwardTimeoutSeconds specifies a timeout per SSH port forward.
 	// Zero value means a port forward will not time out.
-	// If omitted default value is TUNNEL_PORT_FORWARD_DIAL_TIMEOUT_SECONDS.
+	// If omitted, the default value is TUNNEL_PORT_FORWARD_DIAL_TIMEOUT_SECONDS.
 	TunnelPortForwardTimeoutSeconds *int
 
 	// TunnelSshKeepAliveProbeTimeoutSeconds specifies a timeout value for "probe"
 	// SSH keep-alive that is sent upon port forward failure.
 	// Zero value means keep-alive request will not time out.
-	// If omitted default value is TUNNEL_SSH_KEEP_ALIVE_PROBE_TIMEOUT_SECONDS.
+	// If omitted, the default value is TUNNEL_SSH_KEEP_ALIVE_PROBE_TIMEOUT_SECONDS.
 	TunnelSshKeepAliveProbeTimeoutSeconds *int
 
 	// TunnelSshKeepAlivePeriodicTimeoutSeconds specifies a timeout value for regular
 	// SSH keep-alives that are sent periodically.
 	// Zero value means keep-alive request will not time out.
-	// If omitted default value is TUNNEL_SSH_KEEP_ALIVE_PERIODIC_TIMEOUT_SECONDS.
+	// If omitted, the default value is TUNNEL_SSH_KEEP_ALIVE_PERIODIC_TIMEOUT_SECONDS.
 	TunnelSshKeepAlivePeriodicTimeoutSeconds *int
 
 	// FetchRemoteServerListTimeoutSeconds specifies a timeout value for remote server list
 	// HTTP request. Zero value means that request will not time out.
-	// If omitted default value is FETCH_REMOTE_SERVER_LIST_TIMEOUT_SECONDS.
+	// If omitted, the default value is FETCH_REMOTE_SERVER_LIST_TIMEOUT_SECONDS.
 	FetchRemoteServerListTimeoutSeconds *int
 
 	// PsiphonApiServerTimeoutSeconds specifies a timeout for periodic API HTTP
 	// requests to Psiphon server such as stats, home pages, etc.
 	// Zero value means that request will not time out.
-	// If omitted default value is PSIPHON_API_SERVER_TIMEOUT_SECONDS.
+	// If omitted, the default value is PSIPHON_API_SERVER_TIMEOUT_SECONDS.
 	// Note that this value is overridden for final stats requests during shutdown
 	// process in order to prevent hangs.
 	PsiphonApiServerTimeoutSeconds *int
 
 	// FetchRoutesTimeoutSeconds specifies a timeout value for split tunnel routes
 	// HTTP request. Zero value means that request will not time out.
-	// If omitted default value is FETCH_ROUTES_TIMEOUT_SECONDS.
+	// If omitted, the default value is FETCH_ROUTES_TIMEOUT_SECONDS.
 	FetchRoutesTimeoutSeconds *int
 
 	// HttpProxyOriginServerTimeoutSeconds specifies an HTTP response header timeout
 	// value in various HTTP relays found in httpProxy.
 	// Zero value means that request will not time out.
-	// If omitted default value  HTTP_PROXY_ORIGIN_SERVER_TIMEOUT_SECONDS.
+	// If omitted, the default value is HTTP_PROXY_ORIGIN_SERVER_TIMEOUT_SECONDS.
 	HttpProxyOriginServerTimeoutSeconds *int
+
+	// FetchRemoteServerListRetryPeriodSeconds specifies the delay before
+	// resuming a remote server list download after a failure.
+	// If omitted, the default value FETCH_REMOTE_SERVER_LIST_RETRY_PERIOD_SECONDS.
+	FetchRemoteServerListRetryPeriodSeconds *int
+
+	// DownloadUpgradestRetryPeriodSeconds specifies the delay before
+	// resuming a client upgrade download after a failure.
+	// If omitted, the default value DOWNLOAD_UPGRADE_RETRY_PERIOD_SECONDS.
+	DownloadUpgradeRetryPeriodSeconds *int
+
+	// EstablishTunnelPausePeriodSeconds specifies the delay between attempts
+	// to establish tunnels. Briefly pausing allows for network conditions to improve
+	// and for asynchronous operations such as fetch remote server list to complete.
+	// If omitted, the default value is ESTABLISH_TUNNEL_PAUSE_PERIOD_SECONDS.
+	EstablishTunnelPausePeriodSeconds *int
 }
 
 // LoadConfig parses and validates a JSON format Psiphon config JSON
@@ -480,5 +501,20 @@ func LoadConfig(configJson []byte) (*Config, error) {
 		config.HttpProxyOriginServerTimeoutSeconds = &defaultHttpProxyOriginServerTimeoutSeconds
 	}
 
+	if config.FetchRemoteServerListRetryPeriodSeconds == nil {
+		defaultFetchRemoteServerListRetryPeriodSeconds := FETCH_REMOTE_SERVER_LIST_RETRY_PERIOD_SECONDS
+		config.FetchRemoteServerListRetryPeriodSeconds = &defaultFetchRemoteServerListRetryPeriodSeconds
+	}
+
+	if config.DownloadUpgradeRetryPeriodSeconds == nil {
+		defaultDownloadUpgradeRetryPeriodSeconds := DOWNLOAD_UPGRADE_RETRY_PERIOD_SECONDS
+		config.DownloadUpgradeRetryPeriodSeconds = &defaultDownloadUpgradeRetryPeriodSeconds
+	}
+
+	if config.EstablishTunnelPausePeriodSeconds == nil {
+		defaultEstablishTunnelPausePeriodSeconds := ESTABLISH_TUNNEL_PAUSE_PERIOD_SECONDS
+		config.EstablishTunnelPausePeriodSeconds = &defaultEstablishTunnelPausePeriodSeconds
+	}
+
 	return &config, nil
 }

psiphon/controller.go

@@ -248,6 +248,15 @@ func (controller *Controller) SignalComponentFailure() {
 func (controller *Controller) remoteServerListFetcher() {
 	defer controller.runWaitGroup.Done()
 
+	if controller.config.RemoteServerListUrl == "" {
+		NoticeAlert("remote server list URL is blank")
+		return
+	}
+	if controller.config.RemoteServerListSignaturePublicKey == "" {
+		NoticeAlert("remote server list signature public key blank")
+		return
+	}
+
 	var lastFetchTime time.Time
 
 fetcherLoop:
@@ -291,7 +300,8 @@ fetcherLoop:
 
 			NoticeAlert("failed to fetch remote server list: %s", err)
 
-			timeout := time.After(FETCH_REMOTE_SERVER_LIST_RETRY_PERIOD)
+			timeout := time.After(
+				time.Duration(*controller.config.FetchRemoteServerListRetryPeriodSeconds) * time.Second)
 			select {
 			case <-timeout:
 			case <-controller.shutdownBroadcast:
@@ -458,7 +468,8 @@ downloadLoop:
 
 			NoticeAlert("failed to download upgrade: %s", err)
 
-			timeout := time.After(DOWNLOAD_UPGRADE_RETRY_PERIOD)
+			timeout := time.After(
+				time.Duration(*controller.config.DownloadUpgradeRetryPeriodSeconds) * time.Second)
 			select {
 			case <-timeout:
 			case <-controller.shutdownBroadcast:
@@ -1038,7 +1049,8 @@ loop:
 		// network conditions to change. Also allows for fetch remote to complete,
 		// in typical conditions (it isn't strictly necessary to wait for this, there will
 		// be more rounds if required).
-		timeout := time.After(ESTABLISH_TUNNEL_PAUSE_PERIOD)
+		timeout := time.After(
+			time.Duration(*controller.config.EstablishTunnelPausePeriodSeconds) * time.Second)
 		select {
 		case <-timeout:
 			// Retry iterating

psiphon/controller_test.go

@@ -45,15 +45,34 @@ func TestMain(m *testing.M) {
 	os.Exit(m.Run())
 }
 
-// Note: untunneled upgrade tests must execute before
-// the other tests to ensure no tunnel is established.
-// We need a way to reset the datastore after it's been
-// initialized in order to to clear out its data entries
-// and be able to arbitrarily order the tests.
+// Test case notes/limitations/dependencies:
+//
+// * Untunneled upgrade tests must execute before
+//   the other tests to ensure no tunnel is established.
+//   We need a way to reset the datastore after it's been
+//   initialized in order to to clear out its data entries
+//   and be able to arbitrarily order the tests.
+//
+// * The resumable download tests using disruptNetwork
+//   depend on the download object being larger than the
+//   disruptorMax limits so that the disruptor will actually
+//   interrupt the first download attempt. Specifically, the
+//   upgrade and remote server list at the URLs specified in
+//   controller_test.config.enc.
+//
+// * The protocol tests assume there is at least one server
+//   supporting each protocol in the server list at the URL
+//   specified in controller_test.config.enc, and that these
+//   servers are not overloaded.
+//
+// * fetchAndVerifyWebsite depends on the target URL being
+//   available and responding.
+//
 
 func TestUntunneledUpgradeDownload(t *testing.T) {
 	controllerRun(t,
 		&controllerRunConfig{
+			expectNoServerEntries:    true,
 			protocol:                 "",
 			clientIsLatestVersion:    false,
 			disableUntunneledUpgrade: false,
@@ -68,6 +87,7 @@ func TestUntunneledUpgradeDownload(t *testing.T) {
 func TestUntunneledResumableUpgradeDownload(t *testing.T) {
 	controllerRun(t,
 		&controllerRunConfig{
+			expectNoServerEntries:    true,
 			protocol:                 "",
 			clientIsLatestVersion:    false,
 			disableUntunneledUpgrade: false,
@@ -82,6 +102,7 @@ func TestUntunneledResumableUpgradeDownload(t *testing.T) {
 func TestUntunneledUpgradeClientIsLatestVersion(t *testing.T) {
 	controllerRun(t,
 		&controllerRunConfig{
+			expectNoServerEntries:    true,
 			protocol:                 "",
 			clientIsLatestVersion:    true,
 			disableUntunneledUpgrade: false,
@@ -93,9 +114,25 @@ func TestUntunneledUpgradeClientIsLatestVersion(t *testing.T) {
 		})
 }
 
+func TestUntunneledResumableFetchRemoveServerList(t *testing.T) {
+	controllerRun(t,
+		&controllerRunConfig{
+			expectNoServerEntries:    true,
+			protocol:                 "",
+			clientIsLatestVersion:    true,
+			disableUntunneledUpgrade: false,
+			disableEstablishing:      false,
+			tunnelPoolSize:           1,
+			disruptNetwork:           true,
+			useHostNameTransformer:   false,
+			runDuration:              0,
+		})
+}
+
 func TestTunneledUpgradeClientIsLatestVersion(t *testing.T) {
 	controllerRun(t,
 		&controllerRunConfig{
+			expectNoServerEntries:    false,
 			protocol:                 "",
 			clientIsLatestVersion:    true,
 			disableUntunneledUpgrade: true,
@@ -118,6 +155,7 @@ func TestImpairedProtocols(t *testing.T) {
 
 	controllerRun(t,
 		&controllerRunConfig{
+			expectNoServerEntries:    false,
 			protocol:                 "",
 			clientIsLatestVersion:    true,
 			disableUntunneledUpgrade: true,
@@ -132,6 +170,7 @@ func TestImpairedProtocols(t *testing.T) {
 func TestSSH(t *testing.T) {
 	controllerRun(t,
 		&controllerRunConfig{
+			expectNoServerEntries:    false,
 			protocol:                 TUNNEL_PROTOCOL_SSH,
 			clientIsLatestVersion:    false,
 			disableUntunneledUpgrade: true,
@@ -146,6 +185,7 @@ func TestSSH(t *testing.T) {
 func TestObfuscatedSSH(t *testing.T) {
 	controllerRun(t,
 		&controllerRunConfig{
+			expectNoServerEntries:    false,
 			protocol:                 TUNNEL_PROTOCOL_OBFUSCATED_SSH,
 			clientIsLatestVersion:    false,
 			disableUntunneledUpgrade: true,
@@ -160,6 +200,7 @@ func TestObfuscatedSSH(t *testing.T) {
 func TestUnfrontedMeek(t *testing.T) {
 	controllerRun(t,
 		&controllerRunConfig{
+			expectNoServerEntries:    false,
 			protocol:                 TUNNEL_PROTOCOL_UNFRONTED_MEEK,
 			clientIsLatestVersion:    false,
 			disableUntunneledUpgrade: true,
@@ -174,6 +215,7 @@ func TestUnfrontedMeek(t *testing.T) {
 func TestUnfrontedMeekWithTransformer(t *testing.T) {
 	controllerRun(t,
 		&controllerRunConfig{
+			expectNoServerEntries:    false,
 			protocol:                 TUNNEL_PROTOCOL_UNFRONTED_MEEK,
 			clientIsLatestVersion:    true,
 			disableUntunneledUpgrade: true,
@@ -188,6 +230,7 @@ func TestUnfrontedMeekWithTransformer(t *testing.T) {
 func TestFrontedMeek(t *testing.T) {
 	controllerRun(t,
 		&controllerRunConfig{
+			expectNoServerEntries:    false,
 			protocol:                 TUNNEL_PROTOCOL_FRONTED_MEEK,
 			clientIsLatestVersion:    false,
 			disableUntunneledUpgrade: true,
@@ -202,6 +245,7 @@ func TestFrontedMeek(t *testing.T) {
 func TestFrontedMeekWithTransformer(t *testing.T) {
 	controllerRun(t,
 		&controllerRunConfig{
+			expectNoServerEntries:    false,
 			protocol:                 TUNNEL_PROTOCOL_FRONTED_MEEK,
 			clientIsLatestVersion:    true,
 			disableUntunneledUpgrade: true,
@@ -216,6 +260,7 @@ func TestFrontedMeekWithTransformer(t *testing.T) {
 func TestFrontedMeekHTTP(t *testing.T) {
 	controllerRun(t,
 		&controllerRunConfig{
+			expectNoServerEntries:    false,
 			protocol:                 TUNNEL_PROTOCOL_FRONTED_MEEK_HTTP,
 			clientIsLatestVersion:    true,
 			disableUntunneledUpgrade: true,
@@ -230,6 +275,7 @@ func TestFrontedMeekHTTP(t *testing.T) {
 func TestUnfrontedMeekHTTPS(t *testing.T) {
 	controllerRun(t,
 		&controllerRunConfig{
+			expectNoServerEntries:    false,
 			protocol:                 TUNNEL_PROTOCOL_UNFRONTED_MEEK_HTTPS,
 			clientIsLatestVersion:    false,
 			disableUntunneledUpgrade: true,
@@ -244,6 +290,7 @@ func TestUnfrontedMeekHTTPS(t *testing.T) {
 func TestUnfrontedMeekHTTPSWithTransformer(t *testing.T) {
 	controllerRun(t,
 		&controllerRunConfig{
+			expectNoServerEntries:    false,
 			protocol:                 TUNNEL_PROTOCOL_UNFRONTED_MEEK_HTTPS,
 			clientIsLatestVersion:    true,
 			disableUntunneledUpgrade: true,
@@ -256,6 +303,7 @@ func TestUnfrontedMeekHTTPSWithTransformer(t *testing.T) {
 }
 
 type controllerRunConfig struct {
+	expectNoServerEntries    bool
 	protocol                 string
 	clientIsLatestVersion    bool
 	disableUntunneledUpgrade bool
@@ -312,6 +360,14 @@ func controllerRun(t *testing.T, runConfig *controllerRunConfig) {
 		t.Fatalf("error initializing datastore: %s", err)
 	}
 
+	serverEntryCount := CountServerEntries("", "")
+
+	if runConfig.expectNoServerEntries && serverEntryCount > 0 {
+		// TODO: replace expectNoServerEntries with resetServerEntries
+		// so tests can run in arbitrary order
+		t.Fatalf("unexpected server entries")
+	}
+
 	controller, err := NewController(config)
 	if err != nil {
 		t.Fatalf("error creating controller: %s", err)
@@ -326,9 +382,11 @@ func controllerRun(t *testing.T, runConfig *controllerRunConfig) {
 
 	tunnelEstablished := make(chan struct{}, 1)
 	upgradeDownloaded := make(chan struct{}, 1)
+	remoteServerListDownloaded := make(chan struct{}, 1)
 	confirmedLatestVersion := make(chan struct{}, 1)
 
 	var clientUpgradeDownloadedBytesCount int32
+	var remoteServerListDownloadedBytesCount int32
 	var impairedProtocolCount int32
 	var impairedProtocolClassification = struct {
 		sync.RWMutex
@@ -391,6 +449,18 @@ func controllerRun(t *testing.T, runConfig *controllerRunConfig) {
 				default:
 				}
 
+			case "RemoteServerListDownloadedBytes":
+
+				atomic.AddInt32(&remoteServerListDownloadedBytesCount, 1)
+				t.Logf("RemoteServerListDownloadedBytes: %d", int(payload["bytes"].(float64)))
+
+			case "RemoteServerListDownloaded":
+
+				select {
+				case remoteServerListDownloaded <- *new(struct{}):
+				default:
+				}
+
 			case "ImpairedProtocolClassification":
 
 				classification := payload["classification"].(map[string]interface{})
@@ -459,9 +529,9 @@ func controllerRun(t *testing.T, runConfig *controllerRunConfig) {
 
 	if !runConfig.disableEstablishing {
 
-		// Test: tunnel must be established within 60 seconds
+		// Test: tunnel must be established within 120 seconds
 
-		establishTimeout := time.NewTimer(60 * time.Second)
+		establishTimeout := time.NewTimer(120 * time.Second)
 
 		select {
 		case <-tunnelEstablished:
@@ -470,6 +540,25 @@ func controllerRun(t *testing.T, runConfig *controllerRunConfig) {
 			t.Fatalf("tunnel establish timeout exceeded")
 		}
 
+		// Test: if starting with no server entries, a fetch remote
+		// server list must have succeeded. With disruptNetwork, the
+		// fetch must have been resumed at least once.
+
+		if serverEntryCount == 0 {
+			select {
+			case <-remoteServerListDownloaded:
+			default:
+				t.Fatalf("expected remote server list downloaded")
+			}
+
+			if runConfig.disruptNetwork {
+				count := atomic.LoadInt32(&remoteServerListDownloadedBytesCount)
+				if count <= 1 {
+					t.Fatalf("unexpected remote server list download progress: %d", count)
+				}
+			}
+		}
+
 		// Test: fetch website through tunnel
 
 		// Allow for known race condition described in NewHttpProxy():
@@ -508,9 +597,9 @@ func controllerRun(t *testing.T, runConfig *controllerRunConfig) {
 		}
 	}
 
-	// Test: upgrade check/download must be downloaded within 120 seconds
+	// Test: upgrade check/download must be downloaded within 180 seconds
 
-	upgradeTimeout := time.NewTimer(120 * time.Second)
+	upgradeTimeout := time.NewTimer(180 * time.Second)
 
 	select {
 	case <-upgradeDownloaded:
@@ -653,7 +742,7 @@ func useTunnel(t *testing.T, httpProxyPort int) {
 
 const disruptorProxyAddress = "127.0.0.1:2160"
 const disruptorProxyURL = "socks4a://" + disruptorProxyAddress
-const disruptorMaxConnectionBytes = 2000000
+const disruptorMaxConnectionBytes = 625000
 const disruptorMaxConnectionTime = 10 * time.Second
 
 func initDisruptor() {

psiphon/net.go

@@ -413,6 +413,7 @@ func MakeDownloadHttpClient(
 // httpClient, storing the result in downloadFilename when the download is
 // complete. Intermediate, partial downloads state is stored in
 // downloadFilename.part and downloadFilename.part.etag.
+// Any existing downloadFilename file will be overwritten.
 //
 // In the case where the remote object has change while a partial download
 // is to be resumed, the partial state is reset and resumeDownload fails.
@@ -540,20 +541,26 @@ func ResumeDownload(
 	// will fail, leaving a partial download in place (.part and .part.etag).
 	n, err := io.Copy(NewSyncFileWriter(file), response.Body)
 
+	// From this point, n bytes are indicated as downloaded, even if there is
+	// an error; the caller may use this to report partial download progress.
+
 	if err != nil {
-		return 0, "", ContextError(err)
+		return n, "", ContextError(err)
 	}
 
 	// Ensure the file is flushed to disk. The deferred close
 	// will be a noop when this succeeds.
 	err = file.Close()
 	if err != nil {
-		return 0, "", ContextError(err)
+		return n, "", ContextError(err)
 	}
 
+	// Remove if exists, to enable rename
+	os.Remove(downloadFilename)
+
 	err = os.Rename(partialFilename, downloadFilename)
 	if err != nil {
-		return 0, "", ContextError(err)
+		return n, "", ContextError(err)
 	}
 
 	os.Remove(partialETagFilename)

psiphon/notice.go

@@ -328,13 +328,13 @@ func NoticeExiting() {
 
 // NoticeRemoteServerListDownloadedBytes reports remote server list download progress.
 func NoticeRemoteServerListDownloadedBytes(bytes int64) {
-	outputNotice("NoticeRemoteServerListDownloadedBytes", true, false, "bytes", bytes)
+	outputNotice("RemoteServerListDownloadedBytes", true, false, "bytes", bytes)
 }
 
 // NoticeRemoteServerListDownloaded indicates that a remote server list download
 // completed successfully.
-func NoticeRemoteServerListDownloaded() {
-	outputNotice("NoticeRemoteServerListDownloaded", false, false)
+func NoticeRemoteServerListDownloaded(filename string) {
+	outputNotice("RemoteServerListDownloaded", false, false, "filename", filename)
 }
 
 type repetitiveNoticeState struct {

psiphon/remoteServerList.go

@@ -21,7 +21,6 @@ package psiphon
 
 import (
 	"compress/zlib"
-	"errors"
 	"io/ioutil"
 	"os"
 	"strings"
@@ -35,17 +34,10 @@ import (
 func FetchRemoteServerList(
 	config *Config,
 	tunnel *Tunnel,
-	untunneledDialConfig *DialConfig) (err error) {
+	untunneledDialConfig *DialConfig) error {
 
 	NoticeInfo("fetching remote server list")
 
-	if config.RemoteServerListUrl == "" {
-		return ContextError(errors.New("remote server list URL is blank"))
-	}
-	if config.RemoteServerListSignaturePublicKey == "" {
-		return ContextError(errors.New("remote server list signature public key blank"))
-	}
-
 	// Select tunneled or untunneled configuration
 
 	httpClient, requestUrl, err := MakeDownloadHttpClient(
@@ -60,8 +52,11 @@ func FetchRemoteServerList(
 
 	// Proceed with download
 
-	splitPath := strings.Split(config.RemoteServerListUrl, "/")
-	downloadFilename := splitPath[len(splitPath)-1]
+	downloadFilename := config.RemoteServerListDownloadFilename
+	if downloadFilename == "" {
+		splitPath := strings.Split(config.RemoteServerListUrl, "/")
+		downloadFilename = splitPath[len(splitPath)-1]
+	}
 
 	lastETag, err := GetUrlETag(config.RemoteServerListUrl)
 	if err != nil {
@@ -71,18 +66,18 @@ func FetchRemoteServerList(
 	n, responseETag, err := ResumeDownload(
 		httpClient, requestUrl, downloadFilename, lastETag)
 
-	if responseETag == lastETag {
-		// The remote server list is unchanged and no data was downloaded
-		return nil
-	}
-
 	NoticeRemoteServerListDownloadedBytes(n)
 
 	if err != nil {
 		return ContextError(err)
 	}
 
-	NoticeRemoteServerListDownloaded()
+	if responseETag == lastETag {
+		// The remote server list is unchanged and no data was downloaded
+		return nil
+	}
+
+	NoticeRemoteServerListDownloaded(downloadFilename)
 
 	// The downloaded content is a zlib compressed authenticated
 	// data package containing a list of encoded server entries.

psiphon/upgradeDownload.go

@@ -99,7 +99,7 @@ func DownloadUpgrade(
 
 		// Note: if the header is missing, Header.Get returns "" and then
 		// strconv.Atoi returns a parse error.
-		availableClientVersion := response.Header.Get(config.UpgradeDownloadClientVersionHeader)
+		availableClientVersion = response.Header.Get(config.UpgradeDownloadClientVersionHeader)
 		checkAvailableClientVersion, err := strconv.Atoi(availableClientVersion)
 		if err != nil {
 			// If the header is missing or malformed, we can't determine the available