From a797f331ea9751d12606eb814afec1ecabfa4ac3 Mon Sep 17 00:00:00 2001
From: Rob Findley <rfindley@google.com>
Date: Wed, 28 Aug 2024 19:16:13 +0000
Subject: [PATCH] internal/upload: only download the upload config if the mode
 is "on"

This is a second, simpler approach to addressing golang/go#68946, after
CL 607796 was deemed too complicated (and contained a bug!).

Simply check the mode file before downloading the upload config. The
test from CL 607796 are preserved.

For golang/go#68946

Change-Id: I1f11252456df2471e1eafe34e72ca9e369ff8e6a
Reviewed-on: https://go-review.googlesource.com/c/telemetry/+/609275
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
---
 internal/configstore/download.go | 11 +++++++++++
 internal/upload/run.go           | 21 ++++++++++++++++++---
 internal/upload/run_test.go      | 17 ++++++++++++-----
 3 files changed, 41 insertions(+), 8 deletions(-)

diff --git a/internal/configstore/download.go b/internal/configstore/download.go
index a38f371..e60ab7e 100644
--- a/internal/configstore/download.go
+++ b/internal/configstore/download.go
@@ -16,6 +16,7 @@ import (
 	"os"
 	"os/exec"
 	"path/filepath"
+	"sync/atomic"
 
 	"golang.org/x/telemetry/internal/telemetry"
 )
@@ -29,12 +30,22 @@ const (
 // creation flag.
 var needNoConsole = func(cmd *exec.Cmd) {}
 
+var downloads int64
+
+// Downloads reports, for testing purposes, the number of times [Download] has
+// been called.
+func Downloads() int64 {
+	return atomic.LoadInt64(&downloads)
+}
+
 // Download fetches the requested telemetry UploadConfig using "go mod
 // download". If envOverlay is provided, it is appended to the environment used
 // for invoking the go command.
 //
 // The second result is the canonical version of the requested configuration.
 func Download(version string, envOverlay []string) (*telemetry.UploadConfig, string, error) {
+	atomic.AddInt64(&downloads, 1)
+
 	if version == "" {
 		version = "latest"
 	}
diff --git a/internal/upload/run.go b/internal/upload/run.go
index eba13b1..e9c8dc2 100644
--- a/internal/upload/run.go
+++ b/internal/upload/run.go
@@ -112,9 +112,24 @@ func newUploader(rcfg RunConfig) (*uploader, error) {
 	logger := log.New(logWriter, "", log.Ltime|log.Lmicroseconds|log.Lshortfile)
 
 	// Fetch the upload config, if it is not provided.
-	config, configVersion, err := configstore.Download("latest", rcfg.Env)
-	if err != nil {
-		return nil, err
+	var (
+		config        *telemetry.UploadConfig
+		configVersion string
+	)
+
+	if mode, _ := dir.Mode(); mode == "on" {
+		// golang/go#68946: only download the upload config if it will be used.
+		//
+		// TODO(rfindley): This is a narrow change aimed at minimally fixing the
+		// associated bug. In the future, we should read the mode only once during
+		// the upload process.
+		config, configVersion, err = configstore.Download("latest", rcfg.Env)
+		if err != nil {
+			return nil, err
+		}
+	} else {
+		config = &telemetry.UploadConfig{}
+		configVersion = "v0.0.0-0"
 	}
 
 	// Set the start time, if it is not provided.
diff --git a/internal/upload/run_test.go b/internal/upload/run_test.go
index c2cb08e..4231985 100644
--- a/internal/upload/run_test.go
+++ b/internal/upload/run_test.go
@@ -20,6 +20,7 @@ import (
 	"time"
 
 	"golang.org/x/telemetry/counter"
+	"golang.org/x/telemetry/internal/configstore"
 	"golang.org/x/telemetry/internal/configtest"
 	"golang.org/x/telemetry/internal/regtest"
 	"golang.org/x/telemetry/internal/telemetry"
@@ -430,12 +431,13 @@ func TestRun_ModeHandling(t *testing.T) {
 	prog := regtest.NewIncProgram(t, "prog1", "counter")
 
 	tests := []struct {
-		mode        string
-		wantUploads int
+		mode                string
+		wantConfigDownloads int64
+		wantUploads         int
 	}{
-		{"off", 0},
-		{"local", 0},
-		{"on", 1}, // only the second week is uploadable
+		{"off", 0, 0},
+		{"local", 0, 0},
+		{"on", 1, 1}, // only the second week is uploadable
 	}
 	for _, test := range tests {
 		t.Run(test.mode, func(t *testing.T) {
@@ -459,10 +461,15 @@ func TestRun_ModeHandling(t *testing.T) {
 				t.Fatal(err)
 			}
 
+			downloadsBefore := configstore.Downloads()
 			if err := upload.Run(cfg); err != nil {
 				t.Fatal(err)
 			}
 
+			if got := configstore.Downloads() - downloadsBefore; got != test.wantConfigDownloads {
+				t.Errorf("configstore.Download called: %v, want %v", got, test.wantConfigDownloads)
+			}
+
 			uploads := getUploads()
 			if gotUploads := len(uploads); gotUploads != test.wantUploads {
 				t.Fatalf("got %d uploads, want %d", gotUploads, test.wantUploads)