fix: required-version respawning of orchestrion causes build failures

internal/cmd/go.go

@@ -23,7 +23,9 @@
 		Args:            true,
 		SkipFlagParsing: true,
 		Action: func(ctx *cli.Context) error {
-			pin.AutoPinOrchestrion(ctx.Context)
+			if err := pin.AutoPinOrchestrion(ctx.Context, ctx.App.Writer, ctx.App.ErrWriter); err != nil {
+				return cli.Exit(err, -1)
+			}
 
 			if err := goproxy.Run(ctx.Context, ctx.Args().Slice(), goproxy.WithToolexec(binpath.Orchestrion, "toolexec")); err != nil {
 				var exitErr *exec.ExitError

internal/cmd/toolexec.go

@@ -50,7 +50,9 @@
 		}
 
 		// Ensure Orchestrion is properly pinned
-		pin.AutoPinOrchestrion(ctx.Context)
+		if err := pin.AutoPinOrchestrion(ctx.Context, ctx.App.Writer, ctx.App.ErrWriter); err != nil {
+			return cli.Exit(err, -1)
+		}
 
 		if proxyCmd.ShowVersion() {
 			log.Trace().Strs("command", proxyCmd.Args()).Msg("Toolexec version command")

internal/cmd/version.go

@@ -9,7 +9,6 @@ import (
 	"fmt"
 	"runtime"
 
-	"github.com/DataDog/orchestrion/internal/ensure"
 	"github.com/DataDog/orchestrion/internal/version"
 	"github.com/urfave/cli/v2"
 )
@@ -40,11 +39,6 @@ var Version = &cli.Command{
 		}
 
 		if c.Bool("verbose") {
-			if startupVersion := ensure.StartupVersion(); startupVersion != version.Tag() {
-				if _, err := fmt.Fprintf(c.App.Writer, " (started as %s)", startupVersion); err != nil {
-					return err
-				}
-			}
 			if _, err := fmt.Fprintf(c.App.Writer, " built with %s (%s/%s)", runtime.Version(), runtime.GOOS, runtime.GOARCH); err != nil {
 				return err
 			}

internal/cmd/version_test.go

@@ -55,12 +55,11 @@ func TestVersion(t *testing.T) {
 
 	t.Run("verbose with respawn", func(t *testing.T) {
 		var output bytes.Buffer
-		t.Setenv("DD_ORCHESTRION_STARTUP_VERSION", "v0.0.0")
 		set := *set
 		set.Parse([]string{"-verbose"})
 		ctx := cli.NewContext(&cli.App{Writer: &output}, &set, nil)
 
 		require.NoError(t, cmd.Version.Action(ctx))
-		require.Equal(t, fmt.Sprintf("orchestrion %s (started as v0.0.0) built with %s (%s/%s)\n", version.Tag(), runtime.Version(), runtime.GOOS, runtime.GOARCH), output.String())
+		require.Equal(t, fmt.Sprintf("orchestrion %s built with %s (%s/%s)\n", version.Tag(), runtime.Version(), runtime.GOOS, runtime.GOARCH), output.String())
 	})
 }

internal/ensure/requiredversion.go

@@ -9,65 +9,55 @@
 	"context"
 	"errors"
 	"fmt"
-	"os"
-	"os/exec"
 	"path/filepath"
 	"runtime"
-	"syscall"
 
 	"github.com/DataDog/orchestrion/internal/goenv"
 	"github.com/DataDog/orchestrion/internal/version"
 	"github.com/rs/zerolog"
 	"golang.org/x/tools/go/packages"
 )
 
-const (
-	orchestrionPkgPath    = "github.com/DataDog/orchestrion"
-	envVarRespawnedFor    = "DD_ORCHESTRION_RESPAWNED_FOR"
-	envVarStartupVersion  = "DD_ORCHESTRION_STARTUP_VERSION"
-	envValRespawnReplaced = "<replaced>"
-)
+const orchestrionPkgPath = "github.com/DataDog/orchestrion"
 
-var (
-	errRespawnLoop    = errors.New("re-spawn loop detected")
-	orchestrionSrcDir string
-)
+var orchestrionSrcDir string
+
+// IncorrectVersionError is returned by [RequiredVersion] when the version of orchestrion running
+// does not match the one required by `go.mod`.
+type IncorrectVersionError struct {
+	// RequiredVersion is the version declared in `go.mod`, or a blank string if a `replace` directive
+	// for "github.com/DataDog/orchestrion" is present in `go.mod`.
+	RequiredVersion string
+}
 
 // RequiredVersion makes sure the version of the tool currently running is the same as the one
-// required in the current working directory's "go.mod" file by calling `syscall.Exec` with the
-// relevant `go run` command if necessary to replace the current process with one using the required
-// version.
+// required in the current working directory's "go.mod" file.
 //
 // If this returns `nil`, the current process is running the correct version of the tool and can
-// proceed with it's intended purpose. If it returns an error, that should be presented to the user
-// before exiting with a non-0 status code. If the process was correctly substituted, this function
-// never returns control to its caller (as the process has been replaced).
+// proceed with it's intended purpose. If it returns an [IncorrectVersionError], the caller should
+// determine whether to print a warning or exit in error, presenting the returned error to the user.
 func RequiredVersion(ctx context.Context) error {
-	return requiredVersion(ctx, goModVersion, os.Getenv, syscall.Exec, os.Args)
+	return requiredVersion(ctx, goModVersion)
 }
 
-// StartupVersion returns the version of Orchestrion that has started this process. If this is the
-// same as version.Tag, this process hasn't needed to be re-started. This is useful to provide
-// complete information about proxied executions (e.g: in the output of `orchestrion version`),
-// in cases where a "globally" installed binary substituted itself for a version from `go.mod`.
-func StartupVersion() string {
-	if env := os.Getenv(envVarStartupVersion); env != "" {
-		return env
+func (e IncorrectVersionError) Error() string {
+	const useMessage = "use `go run github.com/DataDog/orchestrion` instead of just `orchestrion`"
+	if e.RequiredVersion == "" {
+		return "orchestrion is diverted by a replace directive; please " + useMessage
 	}
-	return version.Tag()
+	return fmt.Sprintf(
+		"orchestrion@%s is required by `go.mod`, but this is orchestrion@%s - please run `go install github.com/DataDog/orchestrion@%[1]s` or "+useMessage,
+		e.RequiredVersion,
+		version.Tag(),
+	)
 }
 
 // requiredVersion is the internal implementation of RequiredVersion, and takes the goModVersion and
 // syscall.Exec functions as arguments to allow for easier testing. Panics if `osArgs` is 0-length.
 func requiredVersion(
 	ctx context.Context,
 	goModVersion func(context.Context, string) (string, string, error),
-	osGetenv func(string) string,
-	syscallExec func(argv0 string, argv []string, env []string) error,
-	osArgs []string,
 ) error {
-	log := zerolog.Ctx(ctx)
-
 	rVersion, path, err := goModVersion(ctx, "" /* Current working directory */)
 	if err != nil {
 		return fmt.Errorf("failed to determine go.mod requirement for %q: %w", orchestrionPkgPath, err)
@@ -79,54 +69,7 @@
 		return nil
 	}
 
-	if respawn := osGetenv(envVarRespawnedFor); respawn != "" && respawn != envValRespawnReplaced {
-		// We're already re-spawning for a non-local version, so we should not be re-spawning again...
-		// If that were the case, we'd likely end up in an infinite loop of re-spawning, which is very
-		// much undesirable.
-		return fmt.Errorf(
-			"%w (wanted %s, got %s, already respawning for %s)",
-			errRespawnLoop,
-			rVersion,
-			version.Tag(),
-			respawn,
-		)
-	}
-
-	if rVersion == "" {
-		// If there is no required version, it means a replace directive is in use, and it does not
-		// macth the running process' original source tree, so we will unconditionally re-spawn.
-		rVersion = envValRespawnReplaced
-	}
-
-	log.Info().Msgf("Re-starting with '%s@%s' (this is %s)", orchestrionPkgPath, rVersion, version.Tag())
-
-	goBin, err := exec.LookPath("go")
-	if err != nil {
-		return fmt.Errorf("failed to resolve go from PATH: %w", err)
-	}
-
-	if len(osArgs) == 0 {
-		panic("received 0-length osArgs, which is not supposed to happen")
-	}
-
-	args := make([]string, len(osArgs)+2)
-	args[0] = goBin
-	args[1] = "run"
-	args[2] = orchestrionPkgPath
-	copy(args[3:], osArgs[1:])
-
-	env := os.Environ()
-	env = append(
-		env,
-		fmt.Sprintf("%s=%s", envVarRespawnedFor, rVersion),
-		fmt.Sprintf("%s=%s", envVarStartupVersion, version.Tag()),
-	)
-
-	runtime.LockOSThread()
-	defer runtime.UnlockOSThread()
-
-	// Won't return control if successful, never returns a `nil` error value.
-	return syscallExec(goBin, args, env)
+	return IncorrectVersionError{RequiredVersion: rVersion}
 }
 
 // goModVersion returns the version and path of the "github.com/DataDog/orchestrion" module that is
@@ -175,6 +118,6 @@
 }
 
 func init() {
-	_, file, _, _ := runtime.Caller(0)
-	orchestrionSrcDir = filepath.Dir(filepath.Dir(filepath.Dir(file)))
+	_, thisFile, _, _ := runtime.Caller(0)
+	orchestrionSrcDir = filepath.Join(thisFile, "..", "..", "..")
 }

internal/ensure/requiredversion_test.go

@@ -14,7 +14,6 @@ import (
 	"path/filepath"
 	"runtime"
 	"strings"
-	"sync/atomic"
 	"testing"
 
 	"github.com/DataDog/orchestrion/internal/goenv"
@@ -105,89 +104,55 @@ func TestGoModVersion(t *testing.T) {
 }
 
 func TestRequiredVersion(t *testing.T) {
-	goBin, err := goenv.GoBinPath()
-	require.NoError(t, err, "could not resolve go command path")
-
 	testError := errors.New("simulated failure")
-	osArgs := []string{"/path/to/go/compile", "-a", "./..."}
 
 	type goModVersionResult struct {
 		version string
 		path    string
 		err     error
 	}
-	type expectedOutcome struct {
-		err      error
-		respawns bool
-	}
+	type expectedOutcome = error
 	type testCase struct {
-		goModVersion    goModVersionResult
-		envVarRespawned string
-		expected        expectedOutcome
+		goModVersion goModVersionResult
+		expected     expectedOutcome
 	}
 
 	rawTag, _ := version.TagInfo()
 	for name, tc := range map[string]testCase{
 		"happy path": {
 			goModVersion: goModVersionResult{version: rawTag},
-			expected:     expectedOutcome{err: nil, respawns: false},
+			expected:     nil,
 		},
 		"happy path, replaced to this": {
 			goModVersion: goModVersionResult{path: orchestrionSrcDir},
-			expected:     expectedOutcome{err: nil, respawns: false},
+			expected:     nil,
 		},
 		"go.mod failure": {
 			goModVersion: goModVersionResult{err: testError},
-			expected:     expectedOutcome{err: testError},
+			expected:     testError,
 		},
-		"respawn needed (requires different version)": {
+		"different version required": {
 			goModVersion: goModVersionResult{version: "v1337.42.0-phony.0"},
-			expected:     expectedOutcome{respawns: true},
+			expected:     IncorrectVersionError{RequiredVersion: "v1337.42.0-phony.0"},
 		},
-		"respawn needed (blank required version, blank path)": {
+		"blank version and directory": { // This might never happen in the wild
 			goModVersion: goModVersionResult{},
-			expected:     expectedOutcome{respawns: true},
+			expected:     IncorrectVersionError{RequiredVersion: ""},
 		},
-		"respawn needed (blank required version, mismatched path)": {
+		"replaced to a different path": {
 			goModVersion: goModVersionResult{path: "/phony/orchestrion/path"},
-			expected:     expectedOutcome{respawns: true},
-		},
-		"respawn loop": {
-			goModVersion:    goModVersionResult{version: "v1337.42.0-phony.0"},
-			envVarRespawned: "v1.2.3-example.1",
-			expected:        expectedOutcome{err: errRespawnLoop},
+			expected:     IncorrectVersionError{RequiredVersion: ""},
 		},
 	} {
 		t.Run(name, func(t *testing.T) {
 			mockGoVersion := func(_ context.Context, dir string) (string, string, error) {
 				require.Equal(t, "", dir)
 				return tc.goModVersion.version, tc.goModVersion.path, tc.goModVersion.err
 			}
-			mockGetenv := func(name string) string {
-				require.Equal(t, envVarRespawnedFor, name)
-				return tc.envVarRespawned
-			}
-			var syscallExecCalled atomic.Bool
-			mockSyscallExec := func(arg0 string, args []string, _ []string) error {
-				t.Helper()
-				syscallExecCalled.Store(true)
 
-				require.Equal(t, goBin, arg0)
-				require.GreaterOrEqual(t, len(args), 3)
-				require.Equal(t, []string{goBin, "run", orchestrionPkgPath}, args[:3])
-				require.Equal(t, osArgs[1:], args[3:])
+			err := requiredVersion(context.Background(), mockGoVersion)
 
-				return nil
-			}
-
-			err := requiredVersion(context.Background(), mockGoVersion, mockGetenv, mockSyscallExec, osArgs)
-
-			if tc.expected.err != nil {
-				require.ErrorIs(t, err, tc.expected.err)
-			} else {
-				require.NoError(t, err)
-			}
-			require.Equal(t, tc.expected.respawns, syscallExecCalled.Load())
+			require.ErrorIs(t, err, tc.expected)
 		})
 	}
 }

internal/pin/auto.go

@@ -7,7 +7,9 @@
 
 import (
 	"context"
+	"errors"
 	"fmt"
+	"io"
 	"os"
 	"strings"
 
@@ -21,60 +23,66 @@
 
 const envVarCheckedGoMod = "DD_ORCHESTRION_IS_GOMOD_VERSION"
 
-// AutoPinOrchestrion automatically runs `pinOrchestrion` if the necessary
-// requirements are not already met. It prints messages to `os.Stderr` to inform
+// AutoPinOrchestrion automatically runs [PinOrchestrion] if the necessary
+// requirements are not already met. It prints messages to `stderr` to inform
 // the user about what is going on.
-func AutoPinOrchestrion(ctx context.Context) {
+func AutoPinOrchestrion(ctx context.Context, stdout io.Writer, stderr io.Writer) error {
 	log := zerolog.Ctx(ctx)
 
 	if os.Getenv(envVarCheckedGoMod) == "true" {
 		// A parent process (or ourselves earlier) has already done the check
-		return
+		return nil
 	}
 
 	// Make sure we don't do this again
-	defer func() {
-		_ = os.Setenv(envVarCheckedGoMod, "true")
-	}()
+	if err := os.Setenv(envVarCheckedGoMod, "true"); err != nil {
+		return fmt.Errorf("os.Setenv("+envVarCheckedGoMod+", true): %w", err)
+	}
 
-	var requiredVersionError error
 	if _, isDev := version.TagInfo(); !isDev {
-		requiredVersionError = ensure.RequiredVersion(ctx)
-		if requiredVersionError == nil {
+		err := ensure.RequiredVersion(ctx)
+		if errors.As(err, &ensure.IncorrectVersionError{}) {
+			// There is already a required version, but we're not running that one!
+			log.Trace().Err(err).Msg("Orchestrion is already in go.mod, but we are not running the correct one; returning an error")
+			return err
+		}
+		if err == nil {
 			// We're good to go
-			return
+			log.Trace().Msg("Orchestrion is already in go.mod, and we are running the correct version, no automatic pinning required")
+			return nil
 		}
+		log.Trace().Err(err).Msg("Failed to detect required version of orchestrion from go.mod")
 	} else {
 		log.Trace().Msg("Skipping ensure.RequiredVersion because this is a development build")
 		_, err := os.Stat(config.FilenameOrchestrionToolGo)
 		if err == nil {
 			log.Trace().Msg("Found " + config.FilenameOrchestrionToolGo + " file, no automatic pinning required")
-			return
+			return nil
+		}
+		if !errors.Is(err, os.ErrNotExist) {
+			log.Trace().Err(err).Msg("Failed to stat " + config.FilenameOrchestrionToolGo + ", returning error")
+			return err
 		}
-		requiredVersionError = fmt.Errorf("stat %s: %w", config.FilenameOrchestrionToolGo, err)
+		log.Trace().Msg("No " + config.FilenameOrchestrionToolGo + " file found, will attempt automatic pinning")
 	}
 
-	log.Trace().Err(requiredVersionError).Msg("Failed to detect required version of orchestrion from go.mod")
-
 	var (
 		box       = lipgloss.NewStyle()
 		stylePath = lipgloss.NewStyle()
 		styleFile = lipgloss.NewStyle()
 		styleCmd  = lipgloss.NewStyle()
 	)
-	if term.IsTerminal(int(os.Stderr.Fd())) {
+	if stderr, isFile := stderr.(*os.File); isFile && term.IsTerminal(int(stderr.Fd())) {
 		box = box.Border(lipgloss.RoundedBorder()).
 			BorderForeground(lipgloss.ANSIColor(1)).
 			Padding(1, 2)
-		if w, _, err := term.GetSize(int(os.Stderr.Fd())); err == nil {
+		if w, _, err := term.GetSize(int(stderr.Fd())); err == nil {
 			box.Width(w - box.GetHorizontalMargins() - box.GetHorizontalBorderSize())
 		}
 
 		stylePath = stylePath.Foreground(lipgloss.ANSIColor(4)).Underline(true)
 		styleFile = styleFile.Foreground(lipgloss.ANSIColor(2)).Underline(true)
 		styleCmd = styleCmd.Foreground(lipgloss.ANSIColor(5)).Bold(true).Underline(true)
-	} else {
-		_, _ = fmt.Fprintf(os.Stderr, "Version check error: %v\n", requiredVersionError)
 	}
 
 	var builder strings.Builder
@@ -96,10 +104,11 @@
 	_, _ = builder.WriteString("\n\nYou should commit the resulting changes into your source control system.")
 
 	message := builder.String()
-	_, _ = fmt.Fprintln(os.Stderr, box.Render(message))
+	_, _ = fmt.Fprintln(stderr, box.Render(message))
 
-	if err := PinOrchestrion(ctx, Options{}); err != nil {
-		_, _ = fmt.Fprintf(os.Stderr, "Failed to pin orchestrion in go.mod: %v\n", err)
-		os.Exit(1)
+	if err := PinOrchestrion(ctx, Options{Writer: stdout, ErrWriter: stderr}); err != nil {
+		return fmt.Errorf("failed to pin orchestrion in go.mod: %w", err)
 	}
+
+	return nil
 }

internal/pin/auto_test.go

@@ -7,6 +7,7 @@ package pin
 
 import (
 	"context"
+	"io"
 	"os"
 	"path/filepath"
 	"testing"
@@ -25,7 +26,7 @@ func TestAutoPin(t *testing.T) {
 
 		tmp := scaffold(t, make(map[string]string))
 		require.NoError(t, os.Chdir(tmp))
-		AutoPinOrchestrion(context.Background())
+		AutoPinOrchestrion(context.Background(), io.Discard, io.Discard)
 
 		assert.NotEmpty(t, os.Getenv(envVarCheckedGoMod))
 
@@ -47,7 +48,7 @@ func TestAutoPin(t *testing.T) {
 
 		t.Setenv(envVarCheckedGoMod, "true")
 
-		AutoPinOrchestrion(context.Background())
+		AutoPinOrchestrion(context.Background(), io.Discard, io.Discard)
 
 		assert.NoFileExists(t, filepath.Join(tmp, config.FilenameOrchestrionToolGo))
 		assert.NoFileExists(t, filepath.Join(tmp, "go.sum"))