From 71ebacff261b893e7ac17a07bc37d68ea23fb07e Mon Sep 17 00:00:00 2001 From: Mike Landau Date: Thu, 29 Oct 2026 15:39:48 -0700 Subject: [PATCH 1/2] fix(devbox): make concurrent devbox run safe on shared script files Every `devbox run` rewrote .devbox/gen/scripts in place, and the cleanup in WriteScriptsToFiles deleted .cmd.sh because it was never in the written set. Concurrent runs in the same project could therefore execute a deleted (127), not yet executable (126), or empty (no output) script. Scripts are now written to a temp file and renamed into place, the cleanup keeps .cmd.sh, and it only removes .sh files so it never touches another writer's temp file. Fixes #3025 Co-Authored-By: Claude Opus 5.5 (1M context) --- internal/devbox/devbox.go | 7 ++-- internal/devbox/run_test.go | 72 ++++++++++++++++++++++++++++++++++++ internal/shellgen/scripts.go | 67 +++++++++++++++++---------------- 3 files changed, 108 insertions(+), 38 deletions(-) create mode 100644 internal/devbox/run_test.go diff --git a/internal/devbox/devbox.go b/internal/devbox/devbox.go index 664030f3dad..1856551f4df 100644 --- a/internal/devbox/devbox.go +++ b/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...), " ") } diff --git a/internal/devbox/run_test.go b/internal/devbox/run_test.go new file mode 100644 index 00000000000..715e412dfc6 --- /dev/null +++ b/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 -- ` 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) +} diff --git a/internal/shellgen/scripts.go b/internal/shellgen/scripts.go index 7753b2513e4..f456470f414 100644 --- a/internal/shellgen/scripts.go +++ b/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 { From f1c62285eb611e6ed18a066fe31668bda37617d7 Mon Sep 17 00:00:00 2001 From: Mike Landau Date: Thu, 29 Oct 2026 15:40:28 -0700 Subject: [PATCH 2/2] fix(debug): guard the exec time header with sync.Once The new concurrent script test calls WriteScriptsToFiles from several goroutines, and with DEVBOX_PRINT_EXEC_TIME set the unguarded headerPrinted flag is a data race under -race. Co-Authored-By: Claude Opus 5.5 (1M context) --- internal/debug/time.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/debug/time.go b/internal/debug/time.go index c7b76da9b3a..8c945df61e0 100644 --- a/internal/debug/time.go +++ b/internal/debug/time.go @@ -9,6 +9,7 @@ import ( "runtime" "strconv" "strings" + "sync" "time" ) @@ -16,7 +17,7 @@ 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)) }