fix(config): update config when the local version is unusable

internal/config/alt/update.go

@@ -69,11 +69,18 @@ func Update(ctx context.Context, cnf *config.Config, debugLog func(fmt string, i
 		return nil
 	}
 	if newCnfStruct.Metadata.Version != "" {
-		cmp, err := version.Compare(cnf.Metadata.Version, newCnfStruct.Metadata.Version)
-		if err != nil {
+		// An unusable local version cannot say whether the new config is newer, so
+		// treat the config as out of date. Returning an error would be permanent: it
+		// happens before the file is rewritten, so the version stays unusable.
+		if !version.Validate(cnf.Metadata.Version) {
+			debugLog("Ignoring unusable local config version %q", cnf.Metadata.Version)
+		} else if cmp, err := version.Compare(
+			cnf.Metadata.Version, newCnfStruct.Metadata.Version,
+		); err != nil {
+			// Only the new version can fail to parse here, and FetchConfig validates
+			// it first. Do not apply a config whose version cannot be compared.
 			return fmt.Errorf("could not compare config versions: %w", err)
-		}
-		if cmp >= 0 {
+		} else if cmp >= 0 {
 			debugLog("Config is already up to date (version %s)", cnf.Metadata.Version)
 			return nil
 		}

internal/config/alt/update_test.go

@@ -7,6 +7,7 @@ import (
 	"net/http/httptest"
 	"os"
 	"path/filepath"
+	"strconv"
 	"testing"
 	"time"
 
@@ -30,8 +31,7 @@ func TestUpdate(t *testing.T) {
 	require.NoError(t, err)
 
 	// Set up state so that it stays in a temporary directory.
-	err = os.Setenv(cnf.Application.EnvPrefix+"HOME", tempDir)
-	require.NoError(t, err)
+	t.Setenv(cnf.Application.EnvPrefix+"HOME", tempDir)
 
 	// Set up the config to be updated via a test HTTP server.
 	remoteConfig := testConfig
@@ -86,9 +86,12 @@ func TestUpdate(t *testing.T) {
 	resetTimes()
 
 	remoteConfig = append(remoteConfig, []byte("\nmetadata: {version: 1.0.1}")...)
+	// A local version that cannot be parsed says nothing about whether the new
+	// config is newer, so the update proceeds rather than failing.
 	cnf.Metadata.Version = "invalid"
 	err = alt.Update(ctx, cnf, logger)
-	assert.ErrorContains(t, err, "could not compare config versions")
+	assert.NoError(t, err)
+	assert.Contains(t, lastLogged, "Automatically updated config file")
 	resetTimes()
 	cnf.Metadata.Version = "1.0.1"
 	err = alt.Update(ctx, cnf, logger)
@@ -130,3 +133,85 @@ func TestShouldUpdate(t *testing.T) {
 	cnf.Metadata.URL = ""
 	assert.False(t, alt.ShouldUpdate(cnf))
 }
+
+// testConfigWithMetadata copies the test config and appends a metadata section.
+// It copies because testConfig is shared by every test in the package.
+func testConfigWithMetadata(metadata string) []byte {
+	cnf := make([]byte, 0, len(testConfig)+len("\nmetadata: ")+len(metadata))
+	cnf = append(cnf, testConfig...)
+	cnf = append(cnf, "\nmetadata: "...)
+	return append(cnf, metadata...)
+}
+
+// TestUpdateWithUnusableLocalVersion checks that a config with a URL but no
+// usable version is still updated, which is what clears the bad version.
+func TestUpdateWithUnusableLocalVersion(t *testing.T) {
+	for _, localVersion := range []string{"", "invalid", "1.2.3.4"} {
+		t.Run("local version "+strconv.Quote(localVersion), func(t *testing.T) {
+			tempDir := t.TempDir()
+			testConfigFilename := filepath.Join(tempDir, "config.yaml")
+			require.NoError(t, os.WriteFile(testConfigFilename, testConfig, 0o600))
+			hourAgo := time.Now().Add(-time.Hour)
+			require.NoError(t, os.Chtimes(testConfigFilename, hourAgo, hourAgo))
+
+			cnf, err := config.FromYAML(testConfig)
+			require.NoError(t, err)
+			t.Setenv(cnf.Application.EnvPrefix+"HOME", tempDir)
+
+			remoteConfig := testConfigWithMetadata("{version: 1.0.1}")
+			server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+				_, _ = w.Write(remoteConfig)
+			}))
+			defer server.Close()
+
+			cnf.SourceFile = testConfigFilename
+			cnf.Metadata.URL = server.URL + "/config.yaml"
+			cnf.Metadata.Version = localVersion
+
+			var lastLogged string
+			err = alt.Update(config.ToContext(context.Background(), cnf), cnf,
+				func(msg string, args ...any) { lastLogged = fmt.Sprintf(msg, args...) })
+			assert.NoError(t, err)
+			assert.Contains(t, lastLogged, "Automatically updated config file")
+
+			// The new version is now on disk, so the next run compares cleanly.
+			b, err := os.ReadFile(testConfigFilename)
+			require.NoError(t, err)
+			updated, err := config.FromYAML(b)
+			require.NoError(t, err)
+			assert.Equal(t, "1.0.1", updated.Metadata.Version)
+		})
+	}
+}
+
+// TestUpdateWithUnusableNewVersion checks that a served config whose version
+// will not parse is rejected while fetching and never applied.
+func TestUpdateWithUnusableNewVersion(t *testing.T) {
+	tempDir := t.TempDir()
+	testConfigFilename := filepath.Join(tempDir, "config.yaml")
+	localConfig := testConfigWithMetadata("{version: 1.0.0}")
+	require.NoError(t, os.WriteFile(testConfigFilename, localConfig, 0o600))
+	hourAgo := time.Now().Add(-time.Hour)
+	require.NoError(t, os.Chtimes(testConfigFilename, hourAgo, hourAgo))
+
+	cnf, err := config.FromYAML(localConfig)
+	require.NoError(t, err)
+	require.Equal(t, "1.0.0", cnf.Metadata.Version)
+	t.Setenv(cnf.Application.EnvPrefix+"HOME", tempDir)
+
+	remoteConfig := testConfigWithMetadata("{version: not-a-version}")
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		_, _ = w.Write(remoteConfig)
+	}))
+	defer server.Close()
+
+	cnf.SourceFile = testConfigFilename
+	cnf.Metadata.URL = server.URL + "/config.yaml"
+
+	err = alt.Update(config.ToContext(context.Background(), cnf), cnf, func(string, ...any) {})
+	assert.ErrorContains(t, err, "invalid config")
+
+	b, err := os.ReadFile(testConfigFilename)
+	require.NoError(t, err)
+	assert.Equal(t, string(localConfig), string(b), "the local config must not be modified")
+}