fix: treat empty on-disk JSON state files as empty state (#616)

Author: mwbrooks

internal/app/app_client.go

@@ -22,6 +22,7 @@ import (
 	"strings"
 
 	"github.com/slackapi/slack-cli/internal/config"
+	"github.com/slackapi/slack-cli/internal/goutils"
 	"github.com/slackapi/slack-cli/internal/shared/types"
 	"github.com/slackapi/slack-cli/internal/slackerror"
 	"github.com/slackapi/slack-cli/internal/style"
@@ -297,6 +298,22 @@ func (ac *AppClient) readDeployedApps() error {
 		return err
 	}
 
+	// Treat an empty (or whitespace-only) file as "no apps saved yet" rather
+	// than a parse error. This state occurs when a prior write was truncated
+	// but not completed (e.g. a process was interrupted between afero.WriteFile's
+	// O_TRUNC and the actual write). Without this guard, every subsequent CLI
+	// invocation logs ErrUnableToParseJSON on the same file.
+	if goutils.IsEmptyJSON(f) {
+		ac.apps = types.Apps{
+			DeployedApps: map[string]types.App{},
+			LocalApps:    map[string]types.App{},
+		}
+		if err = ac.saveDeployedApps(); err != nil {
+			return err
+		}
+		return nil
+	}
+
 	if err = json.Unmarshal(f, &ac.apps); err != nil {
 		return slackerror.New(slackerror.ErrUnableToParseJSON).
 			WithMessage("Failed to parse contents of deployed apps file").
@@ -371,6 +388,17 @@ func (ac *AppClient) readLocalApps() error {
 		return err
 	}
 
+	// Treat an empty (or whitespace-only) file as "no local apps saved yet"
+	// rather than a parse error. See readDeployedApps for the corresponding
+	// note on why this state occurs.
+	if goutils.IsEmptyJSON(f) {
+		ac.apps.LocalApps = map[string]types.App{}
+		if err = ac.saveLocalApps(); err != nil {
+			return err
+		}
+		return nil
+	}
+
 	err = json.Unmarshal(f, &ac.apps.LocalApps)
 	if err != nil {
 		return slackerror.New(slackerror.ErrUnableToParseJSON).

internal/app/app_client_test.go

@@ -165,6 +165,30 @@ func Test_AppClient_ReadDeployedApps_BrokenAppsJSON(t *testing.T) {
 	assert.Equal(t, err.(*slackerror.Error).Code, slackerror.ErrUnableToParseJSON)
 }
 
+// Test that a zero-byte or whitespace-only apps.json (e.g. from a truncated
+// write after a prior process interrupt) is treated as empty state, not a
+// parse error. This prevents a hot loop of unable_to_parse_json events on
+// every CLI invocation while the file remains empty on disk.
+func Test_AppClient_ReadDeployedApps_EmptyAppsJSON(t *testing.T) {
+	tests := map[string]string{
+		"zero-byte file":       "",
+		"whitespace-only file": "  \n\t\n",
+	}
+	for name, contents := range tests {
+		t.Run(name, func(t *testing.T) {
+			ac, _, _, pathToAppsJSON, _, teardown := setup(t)
+			defer teardown(t)
+			err := afero.WriteFile(ac.fs, pathToAppsJSON, []byte(contents), 0600)
+			require.NoError(t, err)
+			err = ac.readDeployedApps()
+			require.NoError(t, err)
+			// The empty file should be rewritten as a valid empty apps object.
+			f, _ := afero.ReadFile(ac.fs, pathToAppsJSON)
+			assert.Equal(t, "{}", string(f))
+		})
+	}
+}
+
 // Test that pre-existing dev app details get read from apps.dev.json
 func Test_AppClient_ReadDevApps_ExistingAppsJSON(t *testing.T) {
 	ac, _, _, _, pathToDevAppsJSON, teardown := setup(t)
@@ -207,6 +231,29 @@ func Test_AppClient_ReadDevApps_BrokenAppsJSON(t *testing.T) {
 	assert.Equal(t, err.(*slackerror.Error).Code, slackerror.ErrUnableToParseJSON)
 }
 
+// Test that a zero-byte or whitespace-only apps.dev.json (e.g. from a
+// truncated write after a prior process interrupt) is treated as empty
+// state, not a parse error. See Test_AppClient_ReadDeployedApps_EmptyAppsJSON.
+func Test_AppClient_ReadDevApps_EmptyAppsJSON(t *testing.T) {
+	tests := map[string]string{
+		"zero-byte file":       "",
+		"whitespace-only file": "  \n\t\n",
+	}
+	for name, contents := range tests {
+		t.Run(name, func(t *testing.T) {
+			ac, _, _, _, pathToDevAppsJSON, teardown := setup(t)
+			defer teardown(t)
+			err := afero.WriteFile(ac.fs, pathToDevAppsJSON, []byte(contents), 0600)
+			require.NoError(t, err)
+			err = ac.readLocalApps()
+			require.NoError(t, err)
+			// The empty file should be rewritten as a valid empty apps object.
+			f, _ := afero.ReadFile(ac.fs, pathToDevAppsJSON)
+			assert.Equal(t, "{}", string(f))
+		})
+	}
+}
+
 // Test that a team flag config defines the default app name in an empty AppClient
 func Test_AppClient_getDeployedAppTeamDomain_ViaCLIFlag(t *testing.T) {
 	ac, _, _, _, _, teardown := setup(t)

internal/auth/auth.go

@@ -216,6 +216,16 @@ func (c *Client) auths(ctx context.Context) (map[string]types.SlackAuth, error)
 		return auths, err
 	}
 
+	// Treat an empty (or whitespace-only) credentials file as "no credentials
+	// stored" rather than a parse error. This state occurs when a prior write
+	// was truncated but not completed (e.g. a process was interrupted between
+	// afero.WriteFile's O_TRUNC and the actual write). Without this guard,
+	// every subsequent CLI invocation reads the same 0-byte file and logs
+	// ErrUnableToParseJSON in a hot loop.
+	if goutils.IsEmptyJSON(raw) {
+		return types.AuthByTeamID{}, nil
+	}
+
 	err = json.Unmarshal(raw, &auths)
 	if err != nil {
 		return auths, slackerror.New(slackerror.ErrUnableToParseJSON).

internal/auth/auth_test.go

@@ -222,6 +222,30 @@ func Test_AuthGettersAndSetters(t *testing.T) {
 		require.Error(t, err)
 		assert.Equal(t, err.(*slackerror.Error).Code, slackerror.ErrUnableToParseJSON)
 	})
+
+	// A zero-byte or whitespace-only credentials.json (e.g. from a truncated
+	// write after a prior process interrupt) should be treated as empty state
+	// rather than a parse error, otherwise every CLI invocation would log
+	// ErrUnableToParseJSON in a hot loop.
+	t.Run("empty credentials file returns empty auth state, not a parse error", func(t *testing.T) {
+		cases := map[string]string{
+			"zero-byte file":       "",
+			"whitespace-only file": "  \n\t\n",
+		}
+		for name, contents := range cases {
+			t.Run(name, func(t *testing.T) {
+				ctx, authClient := setup(t)
+				dir, err := authClient.config.SystemConfig.SlackConfigDir(ctx)
+				require.NoError(t, err)
+				path := filepath.Join(dir, credentialsFileName)
+				err = afero.WriteFile(authClient.fs, path, []byte(contents), 0o600)
+				require.NoError(t, err)
+				got, err := authClient.auths(ctx)
+				require.NoError(t, err)
+				require.Empty(t, got)
+			})
+		}
+	})
 }
 
 func Test_AuthsRotation(t *testing.T) {

internal/config/project.go

@@ -25,6 +25,7 @@ import (
 	"github.com/google/uuid"
 	"github.com/opentracing/opentracing-go"
 	"github.com/slackapi/slack-cli/internal/cache"
+	"github.com/slackapi/slack-cli/internal/goutils"
 	"github.com/slackapi/slack-cli/internal/shared/types"
 	"github.com/slackapi/slack-cli/internal/slackerror"
 	"github.com/slackapi/slack-cli/internal/style"
@@ -254,6 +255,17 @@ func ReadProjectConfigFile(ctx context.Context, fs afero.Fs, os types.Os) (Proje
 		return projectConfig, err
 	}
 
+	// Treat an empty (or whitespace-only) project config as "no project config
+	// saved yet" rather than a parse error. This state occurs when a prior
+	// write was truncated but not completed (e.g. a process was interrupted
+	// between afero.WriteFile's O_TRUNC and the actual write). Without this
+	// guard, every subsequent CLI invocation reads the same 0-byte file and
+	// logs ErrUnableToParseJSON in a hot loop.
+	if goutils.IsEmptyJSON(projectConfigFileBytes) {
+		projectConfig.Surveys = map[string]SurveyConfig{}
+		return projectConfig, nil
+	}
+
 	err = json.Unmarshal(projectConfigFileBytes, &projectConfig)
 	if err != nil {
 		return projectConfig, slackerror.New(slackerror.ErrUnableToParseJSON).

internal/config/project_test.go

@@ -352,6 +352,38 @@ func Test_ProjectConfig_ReadProjectConfigFile(t *testing.T) {
 		assert.Equal(t, slackerror.ToSlackError(err).Code, slackerror.ErrUnableToParseJSON)
 		assert.Equal(t, slackerror.ToSlackError(err).Message, "Failed to parse contents of project-level config file")
 	})
+
+	// A zero-byte or whitespace-only .slack/config.json (e.g. from a truncated
+	// write after a prior process interrupt) should be treated as empty state
+	// rather than a parse error, otherwise every CLI invocation would log
+	// ErrUnableToParseJSON in a hot loop.
+	t.Run("empty project config file returns default config, not a parse error", func(t *testing.T) {
+		cases := map[string]string{
+			"zero-byte file":       "",
+			"whitespace-only file": "  \n\t\n",
+		}
+		for name, contents := range cases {
+			t.Run(name, func(t *testing.T) {
+				ctx := slackcontext.MockContext(t.Context())
+				fs := slackdeps.NewFsMock()
+				os := slackdeps.NewOsMock()
+
+				os.AddDefaultMocks()
+				addProjectMocks(t, fs)
+
+				projectDirPath, err := GetProjectDirPath(fs, os)
+				require.NoError(t, err)
+				projectConfigFilePath := GetProjectConfigJSONFilePath(projectDirPath)
+
+				err = afero.WriteFile(fs, projectConfigFilePath, []byte(contents), 0600)
+				require.NoError(t, err)
+
+				got, err := ReadProjectConfigFile(ctx, fs, os)
+				require.NoError(t, err)
+				require.NotNil(t, got.Surveys)
+			})
+		}
+	})
 }
 
 func Test_ProjectConfig_WriteProjectConfigFile(t *testing.T) {

internal/config/system.go

@@ -26,6 +26,7 @@ import (
 
 	"github.com/google/uuid"
 	"github.com/opentracing/opentracing-go"
+	"github.com/slackapi/slack-cli/internal/goutils"
 	"github.com/slackapi/slack-cli/internal/shared/types"
 	"github.com/slackapi/slack-cli/internal/slackerror"
 	"github.com/slackapi/slack-cli/internal/style"
@@ -127,6 +128,17 @@ func (c *SystemConfig) UserConfig(ctx context.Context) (*SystemConfig, error) {
 		return &config, err
 	}
 
+	// Treat an empty (or whitespace-only) config file as "no user config saved
+	// yet" rather than a parse error. This state occurs when a prior write was
+	// truncated but not completed (e.g. a process was interrupted between
+	// afero.WriteFile's O_TRUNC and the actual write). Without this guard,
+	// every subsequent CLI invocation reads the same 0-byte file and logs
+	// ErrUnableToParseJSON in a hot loop.
+	if goutils.IsEmptyJSON(configFileBytes) {
+		config.Surveys = map[string]SurveyConfig{}
+		return &config, nil
+	}
+
 	err = json.Unmarshal(configFileBytes, &config)
 	if err != nil {
 		return &config, slackerror.New(slackerror.ErrUnableToParseJSON).

internal/config/system_test.go

@@ -107,6 +107,37 @@ func Test_SystemConfig_UserConfig(t *testing.T) {
 		assert.Equal(t, slackerror.ToSlackError(err).Code, slackerror.ErrUnableToParseJSON)
 		assert.Equal(t, slackerror.ToSlackError(err).Message, "Failed to parse contents of system-level config file")
 	})
+
+	// A zero-byte or whitespace-only ~/.slack/config.json (e.g. from a truncated
+	// write after a prior process interrupt) should be treated as empty state
+	// rather than a parse error, otherwise every CLI invocation would log
+	// ErrUnableToParseJSON in a hot loop.
+	t.Run("empty configuration file returns default config, not a parse error", func(t *testing.T) {
+		cases := map[string]string{
+			"zero-byte file":       "",
+			"whitespace-only file": "  \n\t\n",
+		}
+		for name, contents := range cases {
+			t.Run(name, func(t *testing.T) {
+				ctx := slackcontext.MockContext(t.Context())
+				fs := slackdeps.NewFsMock()
+				os := slackdeps.NewOsMock()
+
+				os.AddDefaultMocks()
+
+				configFilePath := filepath.Join(slackdeps.MockHomeDirectory, configFolderName, configFileName)
+				err := afero.WriteFile(fs, configFilePath, []byte(contents), 0600)
+				require.NoError(t, err)
+
+				systemConfig := NewSystemConfig(fs, os)
+				got, err := systemConfig.UserConfig(ctx)
+
+				require.NoError(t, err)
+				require.NotNil(t, got)
+				require.NotNil(t, got.Surveys)
+			})
+		}
+	})
 }
 
 func Test_Config_SlackConfigDir(t *testing.T) {

internal/goutils/json.go

@@ -84,3 +84,14 @@ func JSONUnmarshal(data []byte, v interface{}) error {
 	}
 	return nil
 }
+
+// IsEmptyJSON returns true when the provided bytes are empty or contain only
+// whitespace. This matches the state left behind when a JSON state file was
+// truncated but not yet rewritten, e.g. after a process was interrupted between
+// afero.WriteFile's O_TRUNC and the actual write. Callers that read on-disk
+// config/auth/app state should treat this case as "empty state" rather than a
+// parse error, otherwise every subsequent CLI invocation logs
+// ErrUnableToParseJSON until the user manually clears the file.
+func IsEmptyJSON(data []byte) bool {
+	return len(bytes.TrimSpace(data)) == 0
+}

internal/goutils/json_test.go

@@ -159,6 +159,26 @@ func Test_JSONMarshalUnescapedIndent(t *testing.T) {
 	}
 }
 
+func Test_IsEmptyJSON(t *testing.T) {
+	for name, tc := range map[string]struct {
+		data     string
+		expected bool
+	}{
+		"nil bytes":               {data: "", expected: true},
+		"empty string":            {data: "", expected: true},
+		"single space":            {data: " ", expected: true},
+		"whitespace mix":          {data: "  \n\t\n", expected: true},
+		"empty JSON object":       {data: "{}", expected: false},
+		"whitespace around JSON":  {data: "  {}  ", expected: false},
+		"JSON payload":            {data: `{"one":"1"}`, expected: false},
+		"invalid JSON is nonzero": {data: "{", expected: false},
+	} {
+		t.Run(name, func(t *testing.T) {
+			assert.Equal(t, tc.expected, IsEmptyJSON([]byte(tc.data)))
+		})
+	}
+}
+
 func Test_UnmarshalJSON(t *testing.T) {
 	type testConfig struct {
 		One string `json:"one,omitempty"`