From abfa11308309aa9e3fdda7c7a9413c56eb23ed40 Mon Sep 17 00:00:00 2001 From: gillespi314 <73313222+gillespi314@users.noreply.github.com> Date: Wed, 26 Jul 2023 14:40:03 -0500 Subject: [PATCH] Disable nudge in case of launch error (#12906) --- changes/issue-12759-nudge-launch-errors | 4 + orbit/pkg/update/nudge.go | 42 ++++++- orbit/pkg/update/nudge_test.go | 146 +++++++++++++++++++++++- 3 files changed, 188 insertions(+), 4 deletions(-) create mode 100644 changes/issue-12759-nudge-launch-errors diff --git a/changes/issue-12759-nudge-launch-errors b/changes/issue-12759-nudge-launch-errors new file mode 100644 index 0000000000..658ab19335 --- /dev/null +++ b/changes/issue-12759-nudge-launch-errors @@ -0,0 +1,4 @@ +- Addressed issue where Orbit repeatedly tries to launch Nudge in the event of a launch error, which + causes Nudge to steal focus from the user's current application. Instead, Nudge will now be disabled + if it encounters a launch error. It will remain disabled until Orbit is restarted or the Nudge app + is updated. diff --git a/orbit/pkg/update/nudge.go b/orbit/pkg/update/nudge.go index 5f5ea4e85f..aa5327a225 100644 --- a/orbit/pkg/update/nudge.go +++ b/orbit/pkg/update/nudge.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" "os" + "os/exec" "path/filepath" "sync" "time" @@ -16,8 +17,10 @@ import ( "github.com/rs/zerolog/log" ) -const nudgeConfigFile = "nudge-config.json" -const nudgeConfigFileMode = os.FileMode(constant.DefaultWorldReadableFileMode) +const ( + nudgeConfigFile = "nudge-config.json" + nudgeConfigFileMode = os.FileMode(constant.DefaultWorldReadableFileMode) +) // NudgeConfigFetcher is a kind of middleware that wraps an OrbitConfigFetcher and a Runner. // It checks the config supplied by the wrapped OrbitConfigFetcher to detects whether the Fleet @@ -30,6 +33,10 @@ type NudgeConfigFetcher struct { // ensures only one command runs at a time, protects access to lastRun cmdMu sync.Mutex lastRun time.Time + + // launchErr is set if Nudge fails to launch. If launchErr is set, we won't try to + // launch Nudge again. + launchErr *nudgeLaunchErr } type NudgeConfigFetcherOptions struct { @@ -196,9 +203,17 @@ func (n *NudgeConfigFetcher) launch() error { // make sure nudge is added as a target and the hashes // are refreshed if err := checkFileHash(meta, nudge.Path); err != nil { + n.launchErr = nil // reset launchErr since we're dealing with a different file return n.setTargetsAndHashes() } + // if we have a prior launch error, we won't try to launch nudge again + if n.launchErr != nil { + log.Info().Msgf("Nudge disabled since %s due to launch error: %v", n.launchErr.timestamp.Format("2006-01-02"), n.launchErr) + n.lastRun = time.Now() + return nil + } + fn := n.opt.runNudgeFn if fn == nil { fn = func(appPath, configPath string) error { @@ -223,6 +238,17 @@ func (n *NudgeConfigFetcher) launch() error { } if err := fn(nudge.DirPath, fmt.Sprintf("file://%s", cfgFile)); err != nil { + if exitErr, ok := err.(*exec.ExitError); ok { + launchErr := &nudgeLaunchErr{ + err: err, + exitCode: exitErr.ExitCode(), + detail: string(exitErr.Stderr), + cfgFile: cfgFile, + timestamp: time.Now(), + } + n.launchErr = launchErr + return fmt.Errorf("opening Nudge with config %q: %w", cfgFile, launchErr) + } return fmt.Errorf("opening Nudge with config %q: %w", cfgFile, err) } @@ -232,3 +258,15 @@ func (n *NudgeConfigFetcher) launch() error { return nil } + +type nudgeLaunchErr struct { + err error + exitCode int + detail string + cfgFile string + timestamp time.Time +} + +func (e *nudgeLaunchErr) Error() string { + return fmt.Sprintf("%v: %s", e.err, e.detail) +} diff --git a/orbit/pkg/update/nudge_test.go b/orbit/pkg/update/nudge_test.go index 61ca7bbe6b..4b18938225 100644 --- a/orbit/pkg/update/nudge_test.go +++ b/orbit/pkg/update/nudge_test.go @@ -2,8 +2,11 @@ package update import ( "encoding/json" + "fmt" "os" + "os/exec" "path/filepath" + "strings" "testing" "time" @@ -56,10 +59,24 @@ func (s *nudgeTestSuite) TestNudgeConfigFetcherAddNudge() { opt: Options{Targets: make(map[string]TargetInfo), RootDirectory: tmpDir}, } runner := &Runner{updater: updater, localHashes: make(map[string][]byte)} - interval := time.Minute + interval := time.Second cfg := &fleet.OrbitConfig{} nudgePath := "nudge/macos/stable/nudge.app.tar.gz" + + // set up mock runNudgeFn to capture exec command + var execCmd func(command string, args ...string) *exec.Cmd + var execOut string + runNudgeFnInvoked := false runNudgeFn := func(execPath, configPath string) error { + runNudgeFnInvoked = true + if execCmd != nil { + cmd := execCmd(execPath, configPath) + out, err := cmd.Output() + if err != nil { + return err + } + execOut = string(out) + } return nil } @@ -169,7 +186,77 @@ func (s *nudgeTestSuite) TestNudgeConfigFetcherAddNudge() { require.NoError(t, err) require.Equal(t, cfg.NudgeConfig, &savedConfig) - // nudge is removed from targets when the config config is present + // mock exec command to test handling of nudge launch errors + wantCmd := filepath.Join( + tmpDir, + "bin", + "nudge", + NudgeMacOSTarget.Platform, + NudgeMacOSTarget.Channel, + NudgeMacOSTarget.ExtractedExecSubPath[0], + ) + wantArgs := []string{fmt.Sprintf("file://%s", configPath)} + runNudgeFnInvoked = false + + // nudge launches successfully + time.Sleep(1 * time.Second) + execCmd = mockExecCommand(t, "mock stdout", "", wantCmd, wantArgs...) + _, err = f.GetConfig() + require.NoError(t, err) + require.Equal(t, "mock stdout", execOut) + require.True(t, runNudgeFnInvoked) + runNudgeFnInvoked = false + execOut = "" + + // nudge isn't disabled if error is not an ExitError + time.Sleep(1 * time.Second) + execCmd = func(command string, args ...string) *exec.Cmd { + return exec.Command("non-existent-command") + } + _, err = f.GetConfig() + require.ErrorContains(t, err, "exec: \"non-existent-command\": executable file not found in") + require.Empty(t, execOut) + require.True(t, runNudgeFnInvoked) + runNudgeFnInvoked = false + + // nudge launches successfully + time.Sleep(1 * time.Second) + execCmd = mockExecCommand(t, "mock stdout", "", wantCmd, wantArgs...) + _, err = f.GetConfig() + require.NoError(t, err) + require.Equal(t, "mock stdout", execOut) + require.True(t, runNudgeFnInvoked) + runNudgeFnInvoked = false + execOut = "" + + // nudge fails to launch, stderr is captured and logged + time.Sleep(1 * time.Second) + execCmd = mockExecCommand(t, "", "mock stderr", wantCmd, wantArgs...) + _, err = f.GetConfig() + require.ErrorContains(t, err, "exit status 1: mock stderr") + require.Empty(t, execOut) + require.True(t, runNudgeFnInvoked) + runNudgeFnInvoked = false + + // after launch error, nudge will not launch again + time.Sleep(1 * time.Second) + _, err = f.GetConfig() + require.NoError(t, err) + require.Empty(t, execOut) + require.False(t, runNudgeFnInvoked) + time.Sleep(1 * time.Second) + _, err = f.GetConfig() + require.NoError(t, err) + require.Empty(t, execOut) + require.False(t, runNudgeFnInvoked) + time.Sleep(1 * time.Second) + _, err = f.GetConfig() + require.NoError(t, err) + require.NoError(t, err) + require.Empty(t, execOut) + require.False(t, runNudgeFnInvoked) + + // nudge is removed from targets when the config is not present cfg.NudgeConfig = nil gotCfg, err = f.GetConfig() require.NoError(t, err) @@ -180,3 +267,58 @@ func (s *nudgeTestSuite) TestNudgeConfigFetcherAddNudge() { require.False(t, ok) require.Empty(t, ti) } + +// TestHelperProcess is a helper process used for tests that mock exec.Command +// +// Inspired by: https://npf.io/2015/06/testing-exec-command/ +func TestHelperProcess(t *testing.T) { + if os.Getenv("GO_WANT_HELPER_PROCESS") != "1" { + return + } + wantCmd := os.Getenv("GO_WANT_HELPER_PROCESS_COMMAND") + if gotCmd := os.Args[3]; gotCmd != wantCmd { + fmt.Fprint(os.Stderr, fmt.Sprintf("expected command %s but got %s", wantCmd, gotCmd)) + os.Exit(1) + return + } + wantArgs := os.Getenv("GO_WANT_HELPER_PROCESS_ARGS") + if gotArgs := os.Args[4]; gotArgs != wantArgs { + fmt.Fprint(os.Stderr, fmt.Sprintf("expected arg %s but got %s", wantArgs, gotArgs)) + os.Exit(1) + return + } + fmt.Fprintf(os.Stdout, os.Getenv("GO_WANT_HELPER_PROCESS_STDOUT")) + + err := os.Getenv("GO_WANT_HELPER_PROCESS_STDERR") + if err != "" { + fmt.Fprintf(os.Stderr, err) + os.Exit(1) + } + + os.Exit(0) + + return +} + +// mockExecCommand returns a function that can be used to mock exec.Command using TestHelperProcess. +func mockExecCommand(t *testing.T, mockStdout string, mockStderr string, wantCommand string, wantArgs ...string) func(command string, args ...string) *exec.Cmd { + return func(command string, args ...string) *exec.Cmd { + cs := []string{"-test.run=TestHelperProcess", "--", command} + cs = append(cs, args...) + + cmd := exec.Command(os.Args[0], cs...) //nolint:gosec // this is a test helper + cmd.Env = []string{ + "GO_WANT_HELPER_PROCESS=1", + fmt.Sprintf("GO_WANT_HELPER_PROCESS_COMMAND=%s", wantCommand), + fmt.Sprintf("GO_WANT_HELPER_PROCESS_ARGS=%s", strings.Join(wantArgs, " ")), + } + if mockStdout != "" { + cmd.Env = append(cmd.Env, fmt.Sprintf("GO_WANT_HELPER_PROCESS_STDOUT=%s", mockStdout)) + } + if mockStderr != "" { + cmd.Env = append(cmd.Env, fmt.Sprintf("GO_WANT_HELPER_PROCESS_STDERR=%s", mockStderr)) + } + + return cmd + } +}