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

internal/config/alt/update.go

@@ -69,11 +69,21 @@ 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, including an absent one, cannot say whether the
+		// new config is newer, so treat the config as out of date and let the update
+		// below replace it. Returning an error instead would be permanent: it happens
+		// before the file is rewritten, so the unusable version would stay on disk
+		// and every later run would fail the same way.
+		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 before this point. Keep the error rather than applying 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 @@
 	"net/http/httptest"
 	"os"
 	"path/filepath"
+	"strconv"
 	"testing"
 	"time"
 
@@ -30,8 +31,7 @@
 	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 @@
 	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,84 @@
 	cnf.Metadata.URL = ""
 	assert.False(t, alt.ShouldUpdate(cnf))
 }
+
+// TestUpdateWithUnusableLocalVersion covers configs whose local metadata has a
+// URL but no usable version: the update must still be applied. Returning an
+// error here would be permanent, because it happens before the file is
+// rewritten, so the unusable version would stay on disk and every later run
+// would fail identically.
+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 := append(testConfig, []byte("\nmetadata: {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 rewritten file carries the new version, so the next run compares
+			// cleanly instead of repeating this path.
+			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 covers the other side of the comparison: a
+// served config whose version will not parse must never be applied. Validation
+// rejects it while it is being fetched, so Update reports that instead of
+// reaching the version comparison, and the local file is left alone.
+func TestUpdateWithUnusableNewVersion(t *testing.T) {
+	tempDir := t.TempDir()
+	testConfigFilename := filepath.Join(tempDir, "config.yaml")
+	localConfig := append([]byte{}, testConfig...)
+	localConfig = append(localConfig, []byte("\nmetadata: {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 := append([]byte{}, testConfig...)
+	remoteConfig = append(remoteConfig, []byte("\nmetadata: {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")
+}