fix(devbox): make concurrent devbox run safe on shared script files

internal/debug/time.go

@@ -9,14 +9,15 @@ import (
 	"runtime"
 	"strconv"
 	"strings"
+	"sync"
 	"time"
 )
 
 var timerEnabled, _ = strconv.ParseBool(os.Getenv(devboxPrintExecTime))
 
 const devboxPrintExecTime = "DEVBOX_PRINT_EXEC_TIME"
 
-var headerPrinted = false
+var printHeader sync.Once
 
 type timer struct {
 	name string
@@ -49,10 +50,9 @@ func (t *timer) End() {
 	if t == nil {
 		return
 	}
-	if !headerPrinted {
+	printHeader.Do(func() {
 		fmt.Fprintln(os.Stderr, "\nExec times over 1ms:")
-		headerPrinted = true
-	}
+	})
 	if time.Since(t.time) >= time.Millisecond {
 		fmt.Fprintf(os.Stderr, "\"%s\" took %s\n", t.name, time.Since(t.time))
 	}

internal/devbox/devbox.go

@@ -53,8 +53,7 @@ import (
 const (
 
 	// shellHistoryFile keeps the history of commands invoked inside devbox shell
-	shellHistoryFile     = ".devbox/shell_history"
-	arbitraryCmdFilename = ".cmd"
+	shellHistoryFile = ".devbox/shell_history"
 )
 
 type Devbox struct {
@@ -370,11 +369,11 @@ func (d *Devbox) RunScript(ctx context.Context, envOpts devopt.EnvOptions, cmdNa
 		if err != nil {
 			return err
 		}
-		err = shellgen.WriteScriptFile(d, arbitraryCmdFilename, scriptBody)
+		err = shellgen.WriteScriptFile(d, shellgen.ArbitraryCmdFilename, scriptBody)
 		if err != nil {
 			return err
 		}
-		script := shellgen.ScriptPath(d.ProjectDir(), arbitraryCmdFilename)
+		script := shellgen.ScriptPath(d.ProjectDir(), shellgen.ArbitraryCmdFilename)
 		cmdWithArgs = []string{strconv.Quote(script)}
 		env["DEVBOX_RUN_CMD"] = strings.Join(append([]string{runCmd}, cmdArgs...), " ")
 	}

internal/devbox/run_test.go

@@ -0,0 +1,72 @@
+// Copyright 2024 Jetify Inc. and contributors. All rights reserved.
+// Use of this source code is governed by the license in the LICENSE file.
+
+package devbox
+
+import (
+	"fmt"
+	"os"
+	"os/exec"
+	"path/filepath"
+	"sync"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	"go.jetify.com/devbox/internal/devbox/devopt"
+	"go.jetify.com/devbox/internal/shellgen"
+)
+
+// TestConcurrentRunScriptFiles mimics many `devbox run -- <cmd>` invocations
+// in one project: each rewrites the scripts and then executes the shared
+// .cmd.sh. None of them should see a missing, empty, or non-executable script.
+func TestConcurrentRunScriptFiles(t *testing.T) {
+	dir := t.TempDir()
+	config := `{"shell": {"init_hook": ["true"], "scripts": {"hello": "echo hello"}}}`
+	require.NoError(t, os.WriteFile(filepath.Join(dir, "devbox.json"), []byte(config), 0o644))
+	box, err := Open(&devopt.Opts{Dir: dir, Stderr: os.Stderr})
+	require.NoError(t, err)
+
+	cmdBody, err := shellgen.ScriptBody(box, "eval $DEVBOX_RUN_CMD\n")
+	require.NoError(t, err)
+	script := shellgen.ScriptPath(dir, shellgen.ArbitraryCmdFilename)
+
+	const workers, runs = 8, 20
+	errs := make(chan error, workers*runs)
+	var group sync.WaitGroup
+	for worker := range workers {
+		group.Go(func() {
+			for run := range runs {
+				if err := shellgen.WriteScriptsToFiles(box); err != nil {
+					errs <- err
+					continue
+				}
+				if err := shellgen.WriteScriptFile(box, shellgen.ArbitraryCmdFilename, cmdBody); err != nil {
+					errs <- err
+					continue
+				}
+				want := fmt.Sprintf("hello-%d-%d", worker, run)
+				cmd := exec.Command("sh", "-c", script)
+				cmd.Env = append(os.Environ(), "DEVBOX_RUN_CMD=echo "+want)
+				out, err := cmd.CombinedOutput()
+				if err != nil || string(out) != want+"\n" {
+					errs <- fmt.Errorf("run %s: err=%v, output=%q", want, err, out)
+				}
+			}
+		})
+	}
+	group.Wait()
+	close(errs)
+	for err := range errs {
+		t.Error(err)
+	}
+
+	// Concurrent writers shouldn't leave temp files behind.
+	entries, err := os.ReadDir(filepath.Dir(script))
+	require.NoError(t, err)
+	var names []string
+	for _, e := range entries {
+		names = append(names, e.Name())
+	}
+	require.ElementsMatch(t, []string{".cmd.sh", ".hooks.sh", "hello.sh"}, names)
+}

internal/shellgen/scripts.go

@@ -27,6 +27,11 @@ const scriptsDir = ".devbox/gen/scripts"
 
 const HooksFilename = ".hooks"
 
+// ArbitraryCmdFilename is the script `devbox run` uses to run a command that
+// isn't a devbox.json script. Every invocation writes the same contents, so it
+// is shared, and WriteScriptsToFiles must not clean it up.
+const ArbitraryCmdFilename = ".cmd"
+
 type devboxer interface {
 	Config() *devconfig.Config
 	Lockfile() *lock.File
@@ -59,6 +64,8 @@ func WriteScriptsToFiles(devbox devboxer) error {
 		return errors.WithStack(err)
 	}
 	written[HooksFilename] = struct{}{}
+	// Another `devbox run` in this project may be about to execute it.
+	written[ArbitraryCmdFilename] = struct{}{}
 
 	// Write scripts to files.
 	for name, body := range devbox.Config().Scripts() {
@@ -74,9 +81,11 @@ func WriteScriptsToFiles(devbox devboxer) error {
 	}
 
 	// Delete any files that weren't written just now.
+	// Only look at .sh files, so that we skip the temp files of concurrent
+	// writers.
 	for _, entry := range entries {
-		scriptName := strings.TrimSuffix(entry.Name(), ".sh")
-		if _, ok := written[scriptName]; !ok && !entry.IsDir() {
+		scriptName, isScript := strings.CutSuffix(entry.Name(), ".sh")
+		if _, ok := written[scriptName]; !ok && isScript && !entry.IsDir() {
 			err := os.Remove(ScriptPath(devbox.ProjectDir(), scriptName))
 			if err != nil {
 				slog.Debug("failed to clean up script file", "file", entry.Name(), "err", err) // no need to fail run
@@ -87,49 +96,39 @@ func WriteScriptsToFiles(devbox devboxer) error {
 	return nil
 }
 
-func writeRawInitHookFile(devbox devboxer, body string) (err error) {
-	script, err := createScriptFile(devbox, HooksFilename)
-	if err != nil {
-		return errors.WithStack(err)
-	}
-	defer script.Close() // best effort: close file
-
-	_, err = script.WriteString(body)
-	return errors.WithStack(err)
+func writeRawInitHookFile(devbox devboxer, body string) error {
+	return writeScriptFileAtomic(ScriptPath(devbox.ProjectDir(), HooksFilename), body)
 }
 
-func WriteScriptFile(devbox devboxer, name, body string) (err error) {
-	script, err := createScriptFile(devbox, name)
-	if err != nil {
-		return errors.WithStack(err)
-	}
-	defer script.Close() // best effort: close file
-
+func WriteScriptFile(devbox devboxer, name, body string) error {
 	if featureflag.ScriptExitOnError.Enabled() {
 		// NOTE: Devbox scripts run using `sh` for consistency.
 		body = fmt.Sprintf("set -e\n\n%s", body)
 	}
-	_, err = script.WriteString(body)
-	return errors.WithStack(err)
+	return writeScriptFileAtomic(ScriptPath(devbox.ProjectDir(), name), body)
 }
 
-func createScriptFile(devbox devboxer, name string) (script *os.File, err error) {
-	script, err = os.Create(ScriptPath(devbox.ProjectDir(), name))
+// writeScriptFileAtomic writes an executable script to path atomically.
+// Concurrent `devbox run` invocations in the same project rewrite the same
+// scripts, so writing in place could let one of them execute an empty,
+// partial, or not yet executable file.
+func writeScriptFileAtomic(path, body string) error {
+	tmp, err := os.CreateTemp(filepath.Dir(path), "."+filepath.Base(path)+"-*")
 	if err != nil {
-		return nil, errors.WithStack(err)
+		return errors.WithStack(err)
 	}
-	defer func() {
-		// best effort: close file if there was some subsequent error
-		if err != nil {
-			_ = script.Close()
-		}
-	}()
-
-	err = script.Chmod(0o755)
-	if err != nil {
-		return nil, errors.WithStack(err)
+	defer os.Remove(tmp.Name()) // no-op after a successful rename
+	if _, err := tmp.WriteString(body); err != nil {
+		tmp.Close()
+		return errors.WithStack(err)
+	}
+	if err := tmp.Close(); err != nil {
+		return errors.WithStack(err)
+	}
+	if err := os.Chmod(tmp.Name(), 0o755); err != nil {
+		return errors.WithStack(err)
 	}
-	return script, nil
+	return errors.WithStack(os.Rename(tmp.Name(), path))
 }
 
 func ScriptPath(projectDir, scriptName string) string {