Disable nudge in case of launch error (#12906)

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.

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)
+}

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
+	}
+}