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.