From 65daa2d769415f375d1becc54141c5cddb6385b2 Mon Sep 17 00:00:00 2001 From: Mike Landau Date: Sun, 25 Oct 2026 14:34:17 -0700 Subject: [PATCH] fix(devbox): reconcile runx bin dir in place to avoid concurrent races RunXPaths used to RemoveAll the runx bin directory and recreate it on every computeEnv, so parallel devbox commands could delete it out from under each other. Now the directory is reconciled in place: stale entries are removed, missing links are created, and links created concurrently by another process are accepted. Also removes specs/bugs.md now that the deferred bug is fixed. Co-Authored-By: Claude Opus 5.5 (1M context) --- internal/devbox/devbox.go | 63 ++++++++++++++++++++++++++++------ internal/devbox/devbox_test.go | 55 +++++++++++++++++++++++++++++ specs/bugs.md | 36 ------------------- 3 files changed, 108 insertions(+), 46 deletions(-) delete mode 100644 specs/bugs.md diff --git a/internal/devbox/devbox.go b/internal/devbox/devbox.go index 351ed7ba143..8eec2569df1 100644 --- a/internal/devbox/devbox.go +++ b/internal/devbox/devbox.go @@ -1197,13 +1197,10 @@ func (d *Devbox) Lockfile() *lock.File { func (d *Devbox) RunXPaths(ctx context.Context) (string, error) { runxBinPath := filepath.Join(d.projectDir, ".devbox", "virtenv", "runx", "bin") - if err := os.RemoveAll(runxBinPath); err != nil { - return "", err - } - if err := os.MkdirAll(runxBinPath, 0o755); err != nil { - return "", err - } + // Map of symlink name -> target for every runx binary. If two packages + // provide a binary with the same name, the first one wins. + links := map[string]string{} for _, pkg := range d.InstallablePackages() { if !pkg.IsRunX() { continue @@ -1223,17 +1220,63 @@ func (d *Devbox) RunXPaths(ctx context.Context) (string, error) { return "", err } for _, file := range files { - src := filepath.Join(path, file.Name()) - dst := filepath.Join(runxBinPath, file.Name()) - if err := os.Symlink(src, dst); err != nil && !errors.Is(err, os.ErrExist) { - return "", err + if _, ok := links[file.Name()]; !ok { + links[file.Name()] = filepath.Join(path, file.Name()) } } } } + if err := syncSymlinks(runxBinPath, links); err != nil { + return "", err + } return runxBinPath, nil } +// syncSymlinks makes dir contain exactly the symlinks in links (name -> +// target). It reconciles the directory in place instead of recreating it so +// that concurrent devbox processes (and running shells that have dir in their +// PATH) never observe it missing or half-populated. +func syncSymlinks(dir string, links map[string]string) error { + if err := os.MkdirAll(dir, 0o755); err != nil { + return err + } + + entries, err := os.ReadDir(dir) + if err != nil { + return err + } + for _, entry := range entries { + name := entry.Name() + want, ok := links[name] + if ok && isSymlinkTo(filepath.Join(dir, name), want) { + continue + } + err := os.RemoveAll(filepath.Join(dir, name)) + if err != nil && !errors.Is(err, os.ErrNotExist) { + return err + } + } + + for name, target := range links { + dst := filepath.Join(dir, name) + err := os.Symlink(target, dst) + if err == nil { + continue + } + // Another process may have created the link first. + if errors.Is(err, os.ErrExist) && isSymlinkTo(dst, target) { + continue + } + return err + } + return nil +} + +func isSymlinkTo(path, target string) bool { + got, err := os.Readlink(path) + return err == nil && got == target +} + func validateEnvironment(environment string) (string, error) { if environment == "" { return "dev", nil diff --git a/internal/devbox/devbox_test.go b/internal/devbox/devbox_test.go index b0429dfd0d2..dc0f65fc18c 100644 --- a/internal/devbox/devbox_test.go +++ b/internal/devbox/devbox_test.go @@ -132,3 +132,58 @@ func devboxForTesting(t *testing.T) *Devbox { return d } + +func TestSyncSymlinks(t *testing.T) { + dir := filepath.Join(t.TempDir(), "runx", "bin") + require.NoError(t, os.MkdirAll(dir, 0o755)) + + targets := t.TempDir() + target := func(name string) string { return filepath.Join(targets, name) } + + // Pre-existing state: one correct link, one stale link pointing at the + // wrong target, one link that's no longer wanted, and a regular file. + require.NoError(t, os.Symlink(target("correct"), filepath.Join(dir, "correct"))) + require.NoError(t, os.Symlink(target("old"), filepath.Join(dir, "stale"))) + require.NoError(t, os.Symlink(target("removed"), filepath.Join(dir, "removed"))) + require.NoError(t, os.WriteFile(filepath.Join(dir, "file"), nil, 0o644)) + + links := map[string]string{ + "correct": target("correct"), + "stale": target("stale"), + "new": target("new"), + "file": target("file"), + } + require.NoError(t, syncSymlinks(dir, links)) + + entries, err := os.ReadDir(dir) + require.NoError(t, err) + got := map[string]string{} + for _, e := range entries { + dst, err := os.Readlink(filepath.Join(dir, e.Name())) + require.NoError(t, err, "%s should be a symlink", e.Name()) + got[e.Name()] = dst + } + assert.Equal(t, links, got) +} + +func TestSyncSymlinksConcurrent(t *testing.T) { + dir := filepath.Join(t.TempDir(), "runx", "bin") + targets := t.TempDir() + links := map[string]string{} + for i := range 50 { + name := fmt.Sprintf("bin%d", i) + links[name] = filepath.Join(targets, name) + } + + errs := make(chan error, 8) + for range cap(errs) { + go func() { errs <- syncSymlinks(dir, links) }() + } + for range cap(errs) { + assert.NoError(t, <-errs) + } + + entries, err := os.ReadDir(dir) + require.NoError(t, err) + assert.Len(t, entries, len(links)) +} diff --git a/specs/bugs.md b/specs/bugs.md deleted file mode 100644 index b90df7d393c..00000000000 --- a/specs/bugs.md +++ /dev/null @@ -1,36 +0,0 @@ -# Known bugs deferred from the bug-fix sweep - -These bugs were found during an audit of the devbox codebase. Their fixes were -too large to bundle into the bug-fix PRs, so they are tracked here instead. - -## `RunXPaths` races with concurrent devbox commands - -**Where:** `internal/devbox/devbox.go`, `RunXPaths` (around line 1198). - -**Bug:** Every call to `computeEnv` runs `RunXPaths`, which does -`os.RemoveAll(".devbox/virtenv/runx/bin")` and then recreates the directory and -its symlinks. When two devbox commands run at the same time in the same project -(parallel `devbox run`s, or `devbox shellenv` while a shell is open), one -process can delete the directory while the other is creating symlinks in it. -The second process then fails with `ENOENT`/`EEXIST`. Running shells also lose -their runx binaries for the duration of the rebuild, or permanently if the -rebuild fails. - -**Why it isn't a one-liner:** Building into a temp directory and renaming it -into place doesn't work, because `rename(2)` can't replace a non-empty -directory. The fix needs to reconcile the directory in place instead of -recreating it. - -**Fix sketch:** - -1. Build the desired `name → target` map for all runx binaries first. -2. `os.MkdirAll` the `bin` directory. Don't call `RemoveAll`. -3. For each existing entry, remove it only if it's missing from the map or - points to a different target. -4. Create the missing symlinks, treating `fs.ErrExist` as success when the - existing link already points to the right target. This tolerates a - concurrent process creating it first. - -**Estimated size:** one file (`internal/devbox/devbox.go`), about 20–25 lines -changed, plus a unit test that runs the function against a temp project dir -with stale and correct links already present.