Skip to content

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

Merged
mikeland73 merged 2 commits into
mainfrom
mikeland73/fix-run-cmd-race
Oct 30, 2026
Merged

mikeland73 merged 2 commits into
mainfrom
mikeland73/fix-run-cmd-race

Conversation

@mikeland73

Copy link
Copy Markdown
Collaborator

Summary

Fixes #3025. Running devbox run concurrently in one project could fail with exit 127 (.cmd.sh: No such file or directory) or exit 126 (Permission denied), or exit 0 with no output. The code behind this has been there since #550 (2023), so it isn't a regression.

There were two causes:

  1. Cleanup deleted .cmd.sh. WriteScriptsToFiles removes every script it didn't just write, and .cmd was never in that set. Each devbox run deleted the .cmd.sh another run was about to execute.
  2. Scripts were rewritten in place. os.Create truncates the file and then chmods and writes it, so another process could execute it while it was empty or not yet executable.

Changes:

  • All generated scripts (.hooks.sh, the devbox.json scripts, .cmd.sh) are written to a temp file in the same directory and renamed into place, using the same pattern as launcher.writeVersion.
  • The cleanup keeps .cmd.sh and only considers *.sh files, so it never removes another writer's temp file.
  • arbitraryCmdFilename moves to shellgen.ArbitraryCmdFilename so the cleanup can refer to it.
  • internal/debug/time.go: the "Exec times" header flag is now a sync.Once. Without it, the new test trips -race when DEVBOX_PRINT_EXEC_TIME is set.

How was it tested?

  • Added TestConcurrentRunScriptFiles: 8 goroutines × 20 runs, each rewriting the scripts and then executing .cmd.sh. On main it fails with the same exit 127, exit 126 and empty-output cases as the issue. With this change it passes, including with -race.
  • Ran the reproducer from the issue (8 workers, 200 runs, empty devbox.json): installed 0.19.0 had 57/200 failures; a build of this branch had 0/200.
  • go test ./internal/devbox ./internal/shellgen passes, as do the script, script_exit_on_error, args, quote_escaping, shellception, env and path testscripts. Lint is clean.

Community Contribution License

All community contributions in this pull request are licensed to the project
maintainers under the terms of the
Apache 2 License.

By creating this pull request, I represent that I have the right to license the
contributions to the project maintainers under the Apache 2 License as stated in
the
Community Contribution License.

🤖 Generated with Claude Code

mikeland73 and others added 2 commits October 29, 2026 15:39
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
@mikeland73

Copy link
Copy Markdown
Collaborator Author

@claude ptal

@claude

claude Bot commented Oct 30, 2026 •

Copy link
Copy Markdown

Claude finished @mikeland73's task in 19s —— View job


Review of #3032

LGTM. I read the diff but didn't run the tests myself. I found no blocking issues.

What I checked

  • Cleanup fix (internal/shellgen/scripts.go): adding .cmd to written stops WriteScriptsToFiles from deleting .cmd.sh under another run. strings.CutSuffix with the isScript check restricts cleanup to *.sh files.
  • Temp file names: "."+base+"-*" gives names like ..cmd.sh-123456. They don't end in .sh, so the cleanup loop never sees another writer's temp file.
  • Atomic write: write, close, chmod, then rename in the same directory, so a reader sees either the old complete file or the new complete one. Renaming over a file that another process is executing is safe on Linux and macOS. The defer os.Remove is a no-op after a successful rename and cleans up on error paths.
  • Permissions: the final mode is still 0o755, set explicitly, so the umask doesn't change it. Setting it before the rename means the file is never visible without the executable bit.
  • debug/time.go: replacing the bool with sync.Once is correct and fixes the race the new test would otherwise trigger under -race.
  • Test: it covers the real failure modes (127, 126, empty output). The final ElementsMatch on the directory listing also checks that no temp files are left behind.

Minor, non-blocking

  1. If a process is killed between CreateTemp and Rename, a ..<name>.sh-* file stays in the scripts directory. The cleanup loop deliberately ignores these, so they would never be removed. They are rare and tiny, so I'd accept that. If you want to be thorough, you could delete temp files older than some threshold in the cleanup loop.
  2. tmp.Close() in the write-error branch ignores its error. That's fine, since you're already returning the write error.
  3. The comment on ArbitraryCmdFilename says "Every invocation writes the same contents". That holds for the script body, because the command goes in via DEVBOX_RUN_CMD. It also relies on the body not varying per run, for example with exit_on_error toggled differently between concurrent runs, which is harmless.

Nothing else to flag.

@mikeland73
mikeland73 merged commit 0070537 into main Oct 30, 2026
54 of 56 checks passed
@mikeland73
mikeland73 deleted the mikeland73/fix-run-cmd-race branch October 30, 2026 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Concurrent devbox run commands race on shared .cmd.sh

1 participant