From f06b3065a03585b228f7a5ed93f0f45065930a78 Mon Sep 17 00:00:00 2001 From: Mike Landau Date: Fri, 13 Nov 2026 09:29:03 -0800 Subject: [PATCH 1/5] perf(update): resolve in parallel, refresh only changed packages, single recompute - Resolve packages concurrently in `devbox update` (bounded errgroup), then merge lockfile entries serially in input order. Flakes still warn and continue on failure; versioned packages still abort. - Only pass `--refresh` to `nix build` for packages whose lockfile entry changed. Unchanged packages are built only if missing from the store. - Factor out config-only addToConfig/removeFromConfig so Add replacing a same-canonical-name package and Update migrating legacy packages run ensureStateIsUpToDate once instead of per package. - Remove the post-update `nix flake update` call. Generated flake inputs are locked refs from devbox.lock and flake.lock is deleted whenever flake.nix changes, so it never had anything to update. Co-Authored-By: Claude Opus 5.5 --- internal/devbox/packages.go | 159 +++++++++++++++++--------- internal/devbox/update.go | 178 +++++++++++++++++++---------- internal/devbox/update_test.go | 91 +++++++++++++++ internal/nix/flake_update.go | 22 ---- specs/perf-improvements.md | 11 +- testscripts/update/update.test.txt | 11 +- 6 files changed, 328 insertions(+), 144 deletions(-) delete mode 100644 internal/nix/flake_update.go diff --git a/internal/devbox/packages.go b/internal/devbox/packages.go index 3cc331e55d2..021d9ecaf90 100644 --- a/internal/devbox/packages.go +++ b/internal/devbox/packages.go @@ -86,6 +86,38 @@ func (d *Devbox) Add(ctx context.Context, pkgsNames []string, opts devopt.AddOpt ctx, task := trace.NewTask(ctx, "devboxAdd") defer task.End() + added, err := d.addToConfig(ctx, pkgsNames, opts) + if err != nil { + return err + } + + if err := d.ensureStateIsUpToDate(ctx, install); err != nil { + return usererr.WithUserMessage(err, "There was an error installing nix packages") + } + + if err := d.saveCfg(); err != nil { + return err + } + + return d.printPostAddMessage(ctx, added.pkgs, added.unchangedPackageNames, opts) +} + +// addedPackages is what addToConfig added, for printPostAddMessage. +type addedPackages struct { + pkgs []*devpkg.Package + unchangedPackageNames []string + opts devopt.AddOpts +} + +// addToConfig validates `pkgsNames` and adds them to the in-memory config, +// replacing any package with the same canonical name. It does not install +// anything or save the config; callers run ensureStateIsUpToDate and saveCfg +// once after all config changes are made. +func (d *Devbox) addToConfig( + ctx context.Context, + pkgsNames []string, + opts devopt.AddOpts, +) (*addedPackages, error) { // Track which packages had no changes so we can report that to the user. unchangedPackageNames := []string{} @@ -128,7 +160,7 @@ func (d *Devbox) Add(ctx context.Context, pkgsNames []string, opts devopt.AddOpt // Search kept failing even after retries, so we don't know whether the // package exists. Don't silently fall back to nixpkgs (which would drop // the version); tell the user and show how to opt into it. - return d.searchUnavailableError(pkg, err) + return nil, d.searchUnavailableError(pkg, err) } else if errors.Is(err, nix.ErrPackageNotFound) && versionedPkg.IsDevboxPackage && !strings.Contains(pkg.CanonicalName(), ".") { @@ -137,11 +169,11 @@ func (d *Devbox) Add(ctx context.Context, pkgsNames []string, opts devopt.AddOpt // either. Nested attribute paths (e.g. stdenv.cc.cc.lib, openssl.dev, // python3Packages.requests) are not all indexed, so those still fall // back below. - return packageNotFoundError(pkg) + return nil, packageNotFoundError(pkg) } else if !versionedPkg.IsDevboxPackage { // This means it didn't validate and we don't want to fallback to legacy // Just propagate the error. - return err + return nil, err } else { installable := flake.Installable{ Ref: d.lockfile.Stdenv(), @@ -151,7 +183,7 @@ func (d *Devbox) Add(ctx context.Context, pkgsNames []string, opts devopt.AddOpt if err != nil { // This means it looked like a devbox package or attribute path, but we // could not find it in search or in the legacy nixpkgs path. - return packageNotFoundError(pkg) + return nil, packageNotFoundError(pkg) } } @@ -163,8 +195,8 @@ func (d *Devbox) Add(ctx context.Context, pkgsNames []string, opts devopt.AddOpt found, _ := d.findPackageByName(pkg.CanonicalName()) if found != nil { ux.Finfof(d.stderr, "Replacing package %q in devbox.json\n", found.Raw) - if err := d.Remove(ctx, found.Raw); err != nil { - return err + if err := d.removeFromConfig(found.Raw); err != nil { + return nil, err } } @@ -175,18 +207,14 @@ func (d *Devbox) Add(ctx context.Context, pkgsNames []string, opts devopt.AddOpt // Options must be set before ensureStateIsUpToDate. See comment in function if err := d.setPackageOptions(addedPackageNames, opts); err != nil { - return err - } - - if err := d.ensureStateIsUpToDate(ctx, install); err != nil { - return usererr.WithUserMessage(err, "There was an error installing nix packages") - } - - if err := d.saveCfg(); err != nil { - return err + return nil, err } - return d.printPostAddMessage(ctx, pkgs, unchangedPackageNames, opts) + return &addedPackages{ + pkgs: pkgs, + unchangedPackageNames: unchangedPackageNames, + opts: opts, + }, nil } func packageNotFoundError(pkg *devpkg.Package) error { @@ -298,12 +326,29 @@ func (d *Devbox) Remove(ctx context.Context, pkgs ...string) error { ctx, task := trace.NewTask(ctx, "devboxRemove") defer task.End() - packagesToUninstall := []string{} + if err := d.removeFromConfig(pkgs...); err != nil { + return err + } + + // this will clean up the now-extra package from nix profile and the lockfile + if err := d.ensureStateIsUpToDate(ctx, uninstall); err != nil { + return err + } + + return d.saveCfg() +} + +// removeFromConfig removes `pkgs` from the in-memory config and cleans up +// their plugin files. It does not uninstall anything or save the config; +// callers run ensureStateIsUpToDate and saveCfg once after all config changes +// are made. +func (d *Devbox) removeFromConfig(pkgs ...string) error { + removed := []string{} missingPkgs := []string{} for _, pkg := range lo.Uniq(pkgs) { found, _ := d.findPackageByName(pkg) if found != nil { - packagesToUninstall = append(packagesToUninstall, found.Raw) + removed = append(removed, found.Raw) d.cfg.PackageMutator().Remove(found.Raw) } else { missingPkgs = append(missingPkgs, pkg) @@ -318,16 +363,7 @@ func (d *Devbox) Remove(ctx context.Context, pkgs ...string) error { ) } - if err := plugin.Remove(d.projectDir, packagesToUninstall); err != nil { - return err - } - - // this will clean up the now-extra package from nix profile and the lockfile - if err := d.ensureStateIsUpToDate(ctx, uninstall); err != nil { - return err - } - - return d.saveCfg() + return plugin.Remove(d.projectDir, removed) } // installMode is an enum for helping with ensureStateIsUpToDate implementation @@ -611,17 +647,6 @@ func (d *Devbox) installNixPackagesToStore(ctx context.Context, mode installMode return err } - // --no-link to avoid generating the result objects - flags := []string{"--no-link"} - if mode == update { - flags = append(flags, "--refresh") - } - - args := &nix.BuildArgs{ - Flags: flags, - Writer: d.stderr, - } - packageNames := lo.Map( packages, func(p *devpkg.Package, _ int) string { return p.Raw }, @@ -632,32 +657,52 @@ func (d *Devbox) installNixPackagesToStore(ctx context.Context, mode installMode strings.Join(packageNames, ", "), ) - installables := map[bool][]string{false: {}, true: {}} + // Group installables by the flags they need. Only packages whose lockfile + // entry changed in this update are built with --refresh; the rest are + // just missing from the store and don't need to bypass nix's caches. + type buildGroup struct { + allowInsecure bool + refresh bool + } + installables := map[buildGroup][]string{} for _, pkg := range packages { pkgInstallables, err := pkg.Installables() if err != nil { return err } - installables[pkg.HasAllowInsecure()] = append( - installables[pkg.HasAllowInsecure()], - pkgInstallables..., - ) + group := buildGroup{ + allowInsecure: pkg.HasAllowInsecure(), + refresh: mode == update && d.isBeingUpdated(pkg), + } + installables[group] = append(installables[group], pkgInstallables...) } - for allowInsecure, installables := range installables { - if len(installables) == 0 { - continue - } - eventStart := time.Now() - args.AllowInsecure = allowInsecure - err = nix.Build(ctx, args, installables...) - if err != nil { - return err + // Iterate in a fixed order so builds are deterministic. + for _, refresh := range []bool{false, true} { + for _, allowInsecure := range []bool{false, true} { + group := buildGroup{allowInsecure: allowInsecure, refresh: refresh} + if len(installables[group]) == 0 { + continue + } + // --no-link to avoid generating the result objects + flags := []string{"--no-link"} + if refresh { + flags = append(flags, "--refresh") + } + eventStart := time.Now() + err = nix.Build(ctx, &nix.BuildArgs{ + AllowInsecure: allowInsecure, + Flags: flags, + Writer: d.stderr, + }, installables[group]...) + if err != nil { + return err + } + telemetry.Event(telemetry.EventNixBuildSuccess, telemetry.Metadata{ + EventStart: eventStart, + Packages: packageNames, + }) } - telemetry.Event(telemetry.EventNixBuildSuccess, telemetry.Metadata{ - EventStart: eventStart, - Packages: packageNames, - }) } return nil diff --git a/internal/devbox/update.go b/internal/devbox/update.go index 5b991ae25a7..3fa2ed79c8a 100644 --- a/internal/devbox/update.go +++ b/internal/devbox/update.go @@ -18,9 +18,9 @@ import ( "go.jetify.com/devbox/internal/nix" "go.jetify.com/devbox/internal/plugin" "go.jetify.com/devbox/internal/searcher" - "go.jetify.com/devbox/internal/shellgen" "go.jetify.com/devbox/internal/ux" "go.jetify.com/devbox/nix/flake" + "golang.org/x/sync/errgroup" ) func (d *Devbox) Update(ctx context.Context, opts devopt.UpdateOpts) error { @@ -43,39 +43,48 @@ func (d *Devbox) Update(ctx context.Context, opts devopt.UpdateOpts) error { return err } + // Legacy (unversioned) packages are migrated to versioned ones by + // replacing them in the config. This only edits the config; packages are + // installed once by the ensureStateIsUpToDate call below. pendingPackagesToUpdate := []*devpkg.Package{} + var migrated []*addedPackages for _, pkg := range inputs { - if pkg.IsLegacy() { - fmt.Fprintf(d.stderr, "Updating %s -> %s\n", pkg.Raw, pkg.LegacyToVersioned()) + if !pkg.IsLegacy() { + pendingPackagesToUpdate = append(pendingPackagesToUpdate, pkg) + continue + } + fmt.Fprintf(d.stderr, "Updating %s -> %s\n", pkg.Raw, pkg.LegacyToVersioned()) - // Get the package from the config to get the Platforms and ExcludedPlatforms later - cfgPackage, ok := d.cfg.Root.GetPackage(pkg.Raw) - if !ok { - return fmt.Errorf("package %s not found in config", pkg.Raw) - } + // Get the package from the config to get the Platforms and ExcludedPlatforms later + cfgPackage, ok := d.cfg.Root.GetPackage(pkg.Raw) + if !ok { + return fmt.Errorf("package %s not found in config", pkg.Raw) + } - if err := d.Remove(ctx, pkg.Raw); err != nil { - return err - } - // Calling Add function with the original package names, since - // Add will automatically append @latest if search is able to handle that. - // If not, it will fallback to the nixpkg format. - if err := d.Add(ctx, []string{pkg.Raw}, devopt.AddOpts{ - Platforms: cfgPackage.Platforms, - ExcludePlatforms: cfgPackage.ExcludedPlatforms, - }); err != nil { - return err - } - } else { - pendingPackagesToUpdate = append(pendingPackagesToUpdate, pkg) + if err := d.removeFromConfig(pkg.Raw); err != nil { + return err + } + // Calling addToConfig with the original package names, since it will + // automatically append @latest if search is able to handle that. + // If not, it will fallback to the nixpkg format. + added, err := d.addToConfig(ctx, []string{pkg.Raw}, devopt.AddOpts{ + Platforms: cfgPackage.Platforms, + ExcludePlatforms: cfgPackage.ExcludedPlatforms, + }) + if err != nil { + return err } + migrated = append(migrated, added) } - if err := d.updatePendingPackages(pendingPackagesToUpdate); err != nil { + changed, err := d.updatePendingPackages(pendingPackagesToUpdate) + if err != nil { return err } - d.packagesBeingUpdated = inputs + // Only packages whose lockfile entry changed are rebuilt with --refresh. + // Unchanged packages are built only if they're missing from the store. + d.packagesBeingUpdated = changed mode := update if opts.NoInstall { @@ -85,20 +94,24 @@ func (d *Devbox) Update(ctx context.Context, opts devopt.UpdateOpts) error { return err } - // With --no-install, skip the two post-update steps that shell out to Nix: - // "nix flake update" (via FlakeUpdate) and "nix path-info" (via + if len(migrated) > 0 { + if err := d.saveCfg(); err != nil { + return err + } + for _, added := range migrated { + if err := d.printPostAddMessage(ctx, added.pkgs, added.unchangedPackageNames, added.opts); err != nil { + return err + } + } + } + + // With --no-install, skip the post-update "nix path-info" (via // FixMissingStorePaths). This keeps `devbox update --no-install` usable in - // environments where Nix isn't fully set up (e.g. CI/Renovate), where those - // calls would otherwise fail. Earlier steps in the update flow may still - // resolve flake refs via Nix; --no-install only avoids these install-like - // steps. See jetify-com/devbox#2585. + // environments where Nix isn't fully set up (e.g. CI/Renovate), where that + // call would otherwise fail. Earlier steps in the update flow may still + // resolve flake refs via Nix; --no-install only avoids install-like steps. + // See jetify-com/devbox#2585. if !opts.NoInstall { - // I'm not entirely sure this is even needed, so ignoring the error. - // It's definitely not needed for non-flakes. (which is 99.9% of packages) - // It will return an error if .devbox/gen/flake is missing - // TODO: Remove this if it's not needed. - _ = nix.FlakeUpdate(shellgen.FlakePath(d)) - // fix any missing store paths. if err = d.FixMissingStorePaths(ctx); err != nil { return errors.WithStack(err) @@ -128,40 +141,89 @@ func (d *Devbox) inputsToUpdate( return pkgsToUpdate, nil } +// maxConcurrentResolves bounds how many packages `devbox update` resolves at +// once. Each resolve is a search API request or a `nix flake metadata` call. +const maxConcurrentResolves = 8 + // updatePendingPackages updates the lockfile entries for each package, using // the right strategy per package kind. Flake refs warn-and-continue on // failure (see #1180 / #1840); versioned nixpkgs packages abort the update on // failure. Unversioned non-flake entries are left alone. -func (d *Devbox) updatePendingPackages(pkgs []*devpkg.Package) error { - for _, pkg := range pkgs { - if pkgtype.IsFlake(pkg.Raw) { - if err := d.updateDevboxPackage(pkg); err != nil { - ux.Fwarningf(d.stderr, "Failed to update %s: %s\n", pkg.Raw, err) - } +// +// Packages are resolved concurrently, then merged into the lockfile serially +// in input order so output and error behavior match a serial update. It +// returns the packages whose lockfile entry changed. +func (d *Devbox) updatePendingPackages(pkgs []*devpkg.Package) ([]*devpkg.Package, error) { + // refresh=true so flake refs bypass nix's own metadata cache and re-query + // upstream. Without this, `devbox update` on a github: ref can return a + // stale commit that nix had cached from an earlier call. + return d.resolveAndMergePackages(pkgs, func(pkg *devpkg.Package) (*lock.Package, error) { + return d.lockfile.FetchResolvedPackage(pkg.Raw, true) + }) +} + +// resolveAndMergePackages is updatePendingPackages with the resolver injected +// for testing. +func (d *Devbox) resolveAndMergePackages( + pkgs []*devpkg.Package, + fetch func(*devpkg.Package) (*lock.Package, error), +) ([]*devpkg.Package, error) { + type result struct { + resolved *lock.Package + err error + } + results := make([]*result, len(pkgs)) + + var group errgroup.Group + group.SetLimit(maxConcurrentResolves) + for i, pkg := range pkgs { + if !shouldUpdateLockEntry(pkg) { continue } - if _, _, isVersioned := searcher.ParseVersionedPackage(pkg.Raw); isVersioned { - if err := d.updateDevboxPackage(pkg); err != nil { - return err + group.Go(func() error { + resolved, err := fetch(pkg) + results[i] = &result{resolved: resolved, err: err} + return nil + }) + } + _ = group.Wait() // errors are carried in results + + var changed []*devpkg.Package + for i, pkg := range pkgs { + res := results[i] + if res == nil { + continue + } + if res.err != nil { + if pkgtype.IsFlake(pkg.Raw) { + ux.Fwarningf(d.stderr, "Failed to update %s: %s\n", pkg.Raw, res.err) + continue } + return nil, res.err + } + if res.resolved == nil { + continue + } + before := d.lockfile.Packages[pkg.Raw] + if err := d.mergeResolvedPackageToLockfile(pkg, res.resolved, d.lockfile); err != nil { + return nil, err + } + // Merging replaces the entry only when something changed. + if d.lockfile.Packages[pkg.Raw] != before { + changed = append(changed, pkg) } } - return nil + return changed, nil } -func (d *Devbox) updateDevboxPackage(pkg *devpkg.Package) error { - // refresh=true so flake refs bypass nix's own metadata cache and re-query - // upstream. Without this, `devbox update` on a github: ref can return a - // stale commit that nix had cached from an earlier call. - resolved, err := d.lockfile.FetchResolvedPackage(pkg.Raw, true) - if err != nil { - return err - } - if resolved == nil { - return nil +// shouldUpdateLockEntry reports whether `devbox update` re-resolves pkg's +// lockfile entry: flake refs and versioned packages. +func shouldUpdateLockEntry(pkg *devpkg.Package) bool { + if pkgtype.IsFlake(pkg.Raw) { + return true } - - return d.mergeResolvedPackageToLockfile(pkg, resolved, d.lockfile) + _, _, isVersioned := searcher.ParseVersionedPackage(pkg.Raw) + return isVersioned } func (d *Devbox) mergeResolvedPackageToLockfile( diff --git a/internal/devbox/update_test.go b/internal/devbox/update_test.go index 11fa986c106..8c6fa4db329 100644 --- a/internal/devbox/update_test.go +++ b/internal/devbox/update_test.go @@ -1,8 +1,11 @@ package devbox import ( + "errors" + "sync" "testing" + "github.com/samber/lo" "github.com/stretchr/testify/require" "go.jetify.com/devbox/internal/devpkg" "go.jetify.com/devbox/internal/lock" @@ -384,3 +387,91 @@ func TestDescribeFlakeUpdateFormats(t *testing.T) { newNoDate := &lock.Package{Resolved: "github:numtide/flake-utils/" + newRev + "#pkg"} require.Equal(t, "abc1234 -> f456789", describeFlakeUpdate(oldNoDate, newNoDate)) } + +func TestResolveAndMergePackages(t *testing.T) { + devbox := devboxForTesting(t) + + sys := currentSystem(t) + sysInfo := map[string]*lock.SystemInfo{ + sys: {Outputs: []lock.Output{{Name: "out", Path: "/nix/store/x-hello", Default: true}}}, + } + devbox.lockfile.Packages = map[string]*lock.Package{ + "hello@latest": {Version: "1.0", Resolved: "a", Systems: sysInfo}, + "curl@latest": {Version: "8.0", Resolved: "b", Systems: sysInfo}, + } + unchanged := devbox.lockfile.Packages["curl@latest"] + + pkgs := []*devpkg.Package{ + devpkg.PackageFromStringWithDefaults("hello@latest", devbox.lockfile), + devpkg.PackageFromStringWithDefaults("curl@latest", devbox.lockfile), + devpkg.PackageFromStringWithDefaults("github:foo/bar", devbox.lockfile), + devpkg.PackageFromStringWithDefaults("git@latest", devbox.lockfile), + } + + var mu sync.Mutex + var fetched []string + fetch := func(pkg *devpkg.Package) (*lock.Package, error) { + mu.Lock() + fetched = append(fetched, pkg.Raw) + mu.Unlock() + switch pkg.Raw { + case "hello@latest": + return &lock.Package{Version: "2.0", Resolved: "a2", Systems: sysInfo}, nil + case "curl@latest": + return &lock.Package{Version: "8.0", Resolved: "b", Systems: sysInfo}, nil + case "github:foo/bar": + // Flake failures warn and continue. + return nil, errors.New("flake failed") + case "git@latest": + return &lock.Package{Version: "2.40", Resolved: "c"}, nil + } + return nil, errors.New("unexpected package") + } + + changed, err := devbox.resolveAndMergePackages(pkgs, fetch) + require.NoError(t, err) + require.ElementsMatch(t, []string{"hello@latest", "curl@latest", "github:foo/bar", "git@latest"}, fetched) + + changedNames := lo.Map(changed, func(p *devpkg.Package, _ int) string { return p.Raw }) + require.Equal(t, []string{"hello@latest", "git@latest"}, changedNames) + require.Equal(t, "2.0", devbox.lockfile.Packages["hello@latest"].Version) + require.Same(t, unchanged, devbox.lockfile.Packages["curl@latest"]) + require.Equal(t, "2.40", devbox.lockfile.Packages["git@latest"].Version) +} + +func TestResolveAndMergePackagesVersionedErrorAborts(t *testing.T) { + devbox := devboxForTesting(t) + devbox.lockfile.Packages = map[string]*lock.Package{} + + pkgs := []*devpkg.Package{ + devpkg.PackageFromStringWithDefaults("hello@latest", devbox.lockfile), + devpkg.PackageFromStringWithDefaults("curl@latest", devbox.lockfile), + } + wantErr := errors.New("search failed") + fetch := func(pkg *devpkg.Package) (*lock.Package, error) { + if pkg.Raw == "curl@latest" { + return nil, wantErr + } + return &lock.Package{Version: "1.0", Resolved: "a"}, nil + } + + _, err := devbox.resolveAndMergePackages(pkgs, fetch) + require.ErrorIs(t, err, wantErr) +} + +func TestResolveAndMergePackagesSkipsUnversioned(t *testing.T) { + devbox := devboxForTesting(t) + devbox.lockfile.Packages = map[string]*lock.Package{} + + pkgs := []*devpkg.Package{ + devpkg.PackageFromStringWithDefaults("hello", devbox.lockfile), + } + fetch := func(pkg *devpkg.Package) (*lock.Package, error) { + t.Fatalf("unexpected fetch for %s", pkg.Raw) + return nil, nil + } + + changed, err := devbox.resolveAndMergePackages(pkgs, fetch) + require.NoError(t, err) + require.Empty(t, changed) +} diff --git a/internal/nix/flake_update.go b/internal/nix/flake_update.go deleted file mode 100644 index 6eb576c2903..00000000000 --- a/internal/nix/flake_update.go +++ /dev/null @@ -1,22 +0,0 @@ -// Copyright 2024 Jetify Inc. and contributors. All rights reserved. -// Use of this source code is governed by the license in the LICENSE file. - -package nix - -import ( - "context" - "os" - - "go.jetify.com/devbox/internal/ux" - "go.jetify.com/devbox/nix" -) - -func FlakeUpdate(ProfileDir string) error { - ux.Finfof(os.Stderr, "Running \"nix flake update\"\n") - cmd := Command("flake", "update") - if nix.AtLeast(Version2_19) { - cmd.Args = append(cmd.Args, "--flake") - } - cmd.Args = append(cmd.Args, ProfileDir) - return cmd.Run(context.TODO()) -} diff --git a/specs/perf-improvements.md b/specs/perf-improvements.md index 939a43e944d..7095f699bff 100644 --- a/specs/perf-improvements.md +++ b/specs/perf-improvements.md @@ -206,7 +206,7 @@ Fall back to the command for unknown manifest versions. ## Tier 3: `install` / `add` / `update` -### 12. Resolve packages in parallel in `devbox update` +### 12. Resolve packages in parallel in `devbox update` (done in #PRNUM) **Effort:** ~30–50 LOC. @@ -225,7 +225,7 @@ concurrency-safe. Fix: make lockfile resolution safe to call concurrently (lock around lockfile mutation, do the search request outside the lock), then resolve in parallel. Also speeds up `devbox add a b c`. -### 14. Don't `--refresh` unchanged packages on update +### 14. Don't `--refresh` unchanged packages on update (done in #PRNUM) **Effort:** ~20–40 LOC. @@ -243,7 +243,7 @@ installables across packages and run a single `nix path-info` call, then map results back. Keep the per-package error handler for the failure case (retry individually to attribute the error). -### 16. Avoid repeated full recomputes in `Add` / `Update` +### 16. Avoid repeated full recomputes in `Add` / `Update` (done in #PRNUM) **Effort:** ~30–60 LOC. @@ -256,8 +256,9 @@ config-only remove/add, and run a single `ensureStateIsUpToDate` at the end. ### 17. Smaller cleanups Each ~5–20 LOC: -- Remove the `nix.FlakeUpdate` call in `internal/devbox/update.go:100`; its - own comment says it probably isn't needed. +- (done in #PRNUM) Remove the `nix.FlakeUpdate` call in + `internal/devbox/update.go:100`; its own comment says it probably isn't + needed. - Run `nix flake prefetch` once instead of twice (`internal/nix/nixpkgs.go:39-78`): use `--json` on the first call and save the location from its output. diff --git a/testscripts/update/update.test.txt b/testscripts/update/update.test.txt index 156db22ae3d..023adbd693b 100644 --- a/testscripts/update/update.test.txt +++ b/testscripts/update/update.test.txt @@ -3,9 +3,16 @@ exec devbox install exec devbox update +! stderr 'nix flake update' + +# A second update resolves the same lockfile entries, so nothing is rebuilt +# with --refresh. +exec devbox update +stderr 'Already up-to-date hello@latest' +! stderr 'Installing the following packages to the nix store' -# `devbox update --no-install` must not run the post-update "nix flake update" -# step. Regression test for #2585. +# `devbox update --no-install` must not run any post-update Nix steps. +# Regression test for #2585. exec devbox update --no-install ! stderr 'nix flake update' From 4ceab679d45b9a151d8038e6d8e0c9f8b14e3fa3 Mon Sep 17 00:00:00 2001 From: Mike Landau Date: Fri, 13 Nov 2026 09:29:21 -0800 Subject: [PATCH 2/5] docs(spec): mark Tier 3 items 12, 14, 16 done in #3055 Co-Authored-By: Claude Opus 5.5 --- specs/perf-improvements.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/specs/perf-improvements.md b/specs/perf-improvements.md index 7095f699bff..dd97bda7b24 100644 --- a/specs/perf-improvements.md +++ b/specs/perf-improvements.md @@ -206,7 +206,7 @@ Fall back to the command for unknown manifest versions. ## Tier 3: `install` / `add` / `update` -### 12. Resolve packages in parallel in `devbox update` (done in #PRNUM) +### 12. Resolve packages in parallel in `devbox update` (done in #3055) **Effort:** ~30–50 LOC. @@ -225,7 +225,7 @@ concurrency-safe. Fix: make lockfile resolution safe to call concurrently (lock around lockfile mutation, do the search request outside the lock), then resolve in parallel. Also speeds up `devbox add a b c`. -### 14. Don't `--refresh` unchanged packages on update (done in #PRNUM) +### 14. Don't `--refresh` unchanged packages on update (done in #3055) **Effort:** ~20–40 LOC. @@ -243,7 +243,7 @@ installables across packages and run a single `nix path-info` call, then map results back. Keep the per-package error handler for the failure case (retry individually to attribute the error). -### 16. Avoid repeated full recomputes in `Add` / `Update` (done in #PRNUM) +### 16. Avoid repeated full recomputes in `Add` / `Update` (done in #3055) **Effort:** ~30–60 LOC. @@ -256,7 +256,7 @@ config-only remove/add, and run a single `ensureStateIsUpToDate` at the end. ### 17. Smaller cleanups Each ~5–20 LOC: -- (done in #PRNUM) Remove the `nix.FlakeUpdate` call in +- (done in #3055) Remove the `nix.FlakeUpdate` call in `internal/devbox/update.go:100`; its own comment says it probably isn't needed. - Run `nix flake prefetch` once instead of twice From 30b254d603403e2f9d693911f2b2dc9a8d799c4d Mon Sep 17 00:00:00 2001 From: Mike Landau Date: Fri, 13 Nov 2026 09:37:16 -0800 Subject: [PATCH 3/5] fix(update): guard runx registry init, drop lock entry on config remove - RunXRegistry lazily initialized a package-level cache without a lock; parallel resolves in `devbox update` could race on it. Guard it with a mutex. - removeFromConfig now drops the package's lockfile entry. The old Remove path tidied it via ensureStateIsUpToDate; without that, a legacy package re-added under the same name reused its old locked revision. Co-Authored-By: Claude Opus 5.5 --- internal/devbox/packages.go | 7 ++++++- internal/devbox/update_test.go | 16 ++++++++++++++++ internal/devpkg/pkgtype/runx.go | 11 ++++++++++- 3 files changed, 32 insertions(+), 2 deletions(-) diff --git a/internal/devbox/packages.go b/internal/devbox/packages.go index 021d9ecaf90..934fda7e213 100644 --- a/internal/devbox/packages.go +++ b/internal/devbox/packages.go @@ -341,7 +341,7 @@ func (d *Devbox) Remove(ctx context.Context, pkgs ...string) error { // removeFromConfig removes `pkgs` from the in-memory config and cleans up // their plugin files. It does not uninstall anything or save the config; // callers run ensureStateIsUpToDate and saveCfg once after all config changes -// are made. +// are made. It also drops the packages' in-memory lockfile entries. func (d *Devbox) removeFromConfig(pkgs ...string) error { removed := []string{} missingPkgs := []string{} @@ -350,6 +350,11 @@ func (d *Devbox) removeFromConfig(pkgs ...string) error { if found != nil { removed = append(removed, found.Raw) d.cfg.PackageMutator().Remove(found.Raw) + // Drop the lock entry now rather than waiting for the next + // lockfile tidy. If the package is re-added under the same name + // (e.g. a legacy package that falls back to nixpkgs), it must + // be resolved fresh instead of reusing the old revision. + delete(d.lockfile.Packages, found.Raw) } else { missingPkgs = append(missingPkgs, pkg) } diff --git a/internal/devbox/update_test.go b/internal/devbox/update_test.go index 8c6fa4db329..69594a88dd4 100644 --- a/internal/devbox/update_test.go +++ b/internal/devbox/update_test.go @@ -475,3 +475,19 @@ func TestResolveAndMergePackagesSkipsUnversioned(t *testing.T) { require.NoError(t, err) require.Empty(t, changed) } + +func TestRemoveFromConfigDropsLockEntry(t *testing.T) { + devbox := devboxForTesting(t) + devbox.cfg.PackageMutator().Add("hello") + devbox.lockfile.Packages = map[string]*lock.Package{ + "hello": {Resolved: "github:NixOS/nixpkgs/oldrev#hello"}, + } + + require.NoError(t, devbox.removeFromConfig("hello")) + + // A re-added package with the same name must resolve fresh, not reuse + // the old revision. + require.NotContains(t, devbox.lockfile.Packages, "hello") + _, err := devbox.findPackageByName("hello") + require.Error(t, err) +} diff --git a/internal/devpkg/pkgtype/runx.go b/internal/devpkg/pkgtype/runx.go index 2726431dbb7..5d08ebd8af5 100644 --- a/internal/devpkg/pkgtype/runx.go +++ b/internal/devpkg/pkgtype/runx.go @@ -4,6 +4,7 @@ import ( "context" "os" "strings" + "sync" "go.jetify.com/pkg/runx/impl/registry" "go.jetify.com/pkg/runx/impl/runx" @@ -17,7 +18,10 @@ const ( oldGithubAPITokenVarName = "DEVBOX_GITHUB_API_TOKEN" ) -var cachedRegistry *registry.Registry +var ( + cachedRegistry *registry.Registry + cachedRegistryMu sync.Mutex +) func IsRunX(s string) bool { return strings.HasPrefix(s, RunXPrefix) @@ -29,7 +33,12 @@ func RunXClient() *runx.RunX { } } +// RunXRegistry returns the shared runx registry, creating it on first use. It +// is safe for concurrent use (e.g. parallel package resolution in +// `devbox update`). func RunXRegistry(ctx context.Context) (*registry.Registry, error) { + cachedRegistryMu.Lock() + defer cachedRegistryMu.Unlock() if cachedRegistry == nil { var err error cachedRegistry, err = registry.NewLocalRegistry(ctx, getGithubToken()) From d73726105ba65a6ce2b0a305834513d65bac404a Mon Sep 17 00:00:00 2001 From: Mike Landau Date: Fri, 13 Nov 2026 10:38:50 -0800 Subject: [PATCH 4/5] fix(update): only forget lock entry on same-name replacement Deleting the lock entry in removeFromConfig dropped pins that an include still needed. Plain removal now leaves cleanup to lock.File.Tidy again; the entry is forgotten only when a package is removed and re-added under the same name (Add fallback replace, Update legacy migration), which is the case that needs a fresh resolve. Co-Authored-By: Claude Opus 5.5 --- internal/devbox/packages.go | 25 ++++++++++++++++++------- internal/devbox/update.go | 4 ++++ internal/devbox/update_test.go | 25 ++++++++++++++----------- 3 files changed, 36 insertions(+), 18 deletions(-) diff --git a/internal/devbox/packages.go b/internal/devbox/packages.go index 934fda7e213..59d59b97bc0 100644 --- a/internal/devbox/packages.go +++ b/internal/devbox/packages.go @@ -106,7 +106,9 @@ func (d *Devbox) Add(ctx context.Context, pkgsNames []string, opts devopt.AddOpt type addedPackages struct { pkgs []*devpkg.Package unchangedPackageNames []string - opts devopt.AddOpts + // addedNames are the names written to the config. + addedNames []string + opts devopt.AddOpts } // addToConfig validates `pkgsNames` and adds them to the in-memory config, @@ -198,6 +200,9 @@ func (d *Devbox) addToConfig( if err := d.removeFromConfig(found.Raw); err != nil { return nil, err } + if found.Raw == packageNameForConfig { + d.forgetLockEntry(found.Raw) + } } ux.Finfof(d.stderr, "Adding package %q to devbox.json\n", packageNameForConfig) @@ -213,6 +218,7 @@ func (d *Devbox) addToConfig( return &addedPackages{ pkgs: pkgs, unchangedPackageNames: unchangedPackageNames, + addedNames: addedPackageNames, opts: opts, }, nil } @@ -341,7 +347,8 @@ func (d *Devbox) Remove(ctx context.Context, pkgs ...string) error { // removeFromConfig removes `pkgs` from the in-memory config and cleans up // their plugin files. It does not uninstall anything or save the config; // callers run ensureStateIsUpToDate and saveCfg once after all config changes -// are made. It also drops the packages' in-memory lockfile entries. +// are made. Lockfile entries are left for lock.File.Tidy, which keeps ones +// still used by includes. func (d *Devbox) removeFromConfig(pkgs ...string) error { removed := []string{} missingPkgs := []string{} @@ -350,11 +357,6 @@ func (d *Devbox) removeFromConfig(pkgs ...string) error { if found != nil { removed = append(removed, found.Raw) d.cfg.PackageMutator().Remove(found.Raw) - // Drop the lock entry now rather than waiting for the next - // lockfile tidy. If the package is re-added under the same name - // (e.g. a legacy package that falls back to nixpkgs), it must - // be resolved fresh instead of reusing the old revision. - delete(d.lockfile.Packages, found.Raw) } else { missingPkgs = append(missingPkgs, pkg) } @@ -371,6 +373,15 @@ func (d *Devbox) removeFromConfig(pkgs ...string) error { return plugin.Remove(d.projectDir, removed) } +// forgetLockEntry drops pkg's lockfile entry so that a package removed and +// re-added under the same name (e.g. a legacy package that falls back to +// nixpkgs on update) is resolved fresh instead of reusing its old revision. +// Only use it for such replacements: a plain removal must leave the entry for +// lock.File.Tidy, which keeps entries still needed by includes. +func (d *Devbox) forgetLockEntry(pkg string) { + delete(d.lockfile.Packages, pkg) +} + // installMode is an enum for helping with ensureStateIsUpToDate implementation type installMode string diff --git a/internal/devbox/update.go b/internal/devbox/update.go index 3fa2ed79c8a..b52d30c0e0c 100644 --- a/internal/devbox/update.go +++ b/internal/devbox/update.go @@ -74,6 +74,10 @@ func (d *Devbox) Update(ctx context.Context, opts devopt.UpdateOpts) error { if err != nil { return err } + if slices.Contains(added.addedNames, pkg.Raw) { + // Re-added under the same (unversioned) name: resolve it fresh. + d.forgetLockEntry(pkg.Raw) + } migrated = append(migrated, added) } diff --git a/internal/devbox/update_test.go b/internal/devbox/update_test.go index 69594a88dd4..9f3c407541e 100644 --- a/internal/devbox/update_test.go +++ b/internal/devbox/update_test.go @@ -476,18 +476,21 @@ func TestResolveAndMergePackagesSkipsUnversioned(t *testing.T) { require.Empty(t, changed) } -func TestRemoveFromConfigDropsLockEntry(t *testing.T) { +func TestRemoveFromConfigKeepsLockEntry(t *testing.T) { devbox := devboxForTesting(t) - devbox.cfg.PackageMutator().Add("hello") - devbox.lockfile.Packages = map[string]*lock.Package{ - "hello": {Resolved: "github:NixOS/nixpkgs/oldrev#hello"}, - } + devbox.cfg.PackageMutator().Add("hello@latest") + entry := &lock.Package{Resolved: "github:NixOS/nixpkgs/oldrev#hello", Version: "2.10"} + devbox.lockfile.Packages = map[string]*lock.Package{"hello@latest": entry} + + require.NoError(t, devbox.removeFromConfig("hello@latest")) - require.NoError(t, devbox.removeFromConfig("hello")) + _, err := devbox.findPackageByName("hello@latest") + require.Error(t, err, "package should be removed from config") + // Plain removal leaves the entry for lock.File.Tidy, which keeps it if an + // include still needs the package, so its pin isn't lost. + require.Same(t, entry, devbox.lockfile.Packages["hello@latest"]) - // A re-added package with the same name must resolve fresh, not reuse - // the old revision. - require.NotContains(t, devbox.lockfile.Packages, "hello") - _, err := devbox.findPackageByName("hello") - require.Error(t, err) + // A same-name replacement forgets it so the package resolves fresh. + devbox.forgetLockEntry("hello@latest") + require.NotContains(t, devbox.lockfile.Packages, "hello@latest") } From 3e8b648e6e815391af3941170e81bb54394ef1df Mon Sep 17 00:00:00 2001 From: Mike Landau Date: Fri, 13 Nov 2026 10:46:14 -0800 Subject: [PATCH 5/5] refactor(update): extract migrateLegacyPackages to satisfy lint Update's cognitive complexity was 33 (> 32) after the previous changes. Co-Authored-By: Claude Opus 5.5 --- internal/devbox/update.go | 86 +++++++++++++++++++++++---------------- 1 file changed, 50 insertions(+), 36 deletions(-) diff --git a/internal/devbox/update.go b/internal/devbox/update.go index b52d30c0e0c..db679abd0f1 100644 --- a/internal/devbox/update.go +++ b/internal/devbox/update.go @@ -43,42 +43,9 @@ func (d *Devbox) Update(ctx context.Context, opts devopt.UpdateOpts) error { return err } - // Legacy (unversioned) packages are migrated to versioned ones by - // replacing them in the config. This only edits the config; packages are - // installed once by the ensureStateIsUpToDate call below. - pendingPackagesToUpdate := []*devpkg.Package{} - var migrated []*addedPackages - for _, pkg := range inputs { - if !pkg.IsLegacy() { - pendingPackagesToUpdate = append(pendingPackagesToUpdate, pkg) - continue - } - fmt.Fprintf(d.stderr, "Updating %s -> %s\n", pkg.Raw, pkg.LegacyToVersioned()) - - // Get the package from the config to get the Platforms and ExcludedPlatforms later - cfgPackage, ok := d.cfg.Root.GetPackage(pkg.Raw) - if !ok { - return fmt.Errorf("package %s not found in config", pkg.Raw) - } - - if err := d.removeFromConfig(pkg.Raw); err != nil { - return err - } - // Calling addToConfig with the original package names, since it will - // automatically append @latest if search is able to handle that. - // If not, it will fallback to the nixpkg format. - added, err := d.addToConfig(ctx, []string{pkg.Raw}, devopt.AddOpts{ - Platforms: cfgPackage.Platforms, - ExcludePlatforms: cfgPackage.ExcludedPlatforms, - }) - if err != nil { - return err - } - if slices.Contains(added.addedNames, pkg.Raw) { - // Re-added under the same (unversioned) name: resolve it fresh. - d.forgetLockEntry(pkg.Raw) - } - migrated = append(migrated, added) + pendingPackagesToUpdate, migrated, err := d.migrateLegacyPackages(ctx, inputs) + if err != nil { + return err } changed, err := d.updatePendingPackages(pendingPackagesToUpdate) @@ -125,6 +92,53 @@ func (d *Devbox) Update(ctx context.Context, opts devopt.UpdateOpts) error { return plugin.Update() } +// migrateLegacyPackages migrates legacy (unversioned) packages in inputs to +// versioned ones by replacing them in the config. It only edits the config; +// the caller installs packages once with ensureStateIsUpToDate. It returns the +// non-legacy packages, whose lockfile entries still need updating, and what +// was added for each migrated package. +func (d *Devbox) migrateLegacyPackages( + ctx context.Context, + inputs []*devpkg.Package, +) ([]*devpkg.Package, []*addedPackages, error) { + pending := []*devpkg.Package{} + var migrated []*addedPackages + for _, pkg := range inputs { + if !pkg.IsLegacy() { + pending = append(pending, pkg) + continue + } + fmt.Fprintf(d.stderr, "Updating %s -> %s\n", pkg.Raw, pkg.LegacyToVersioned()) + + // Get the package from the config to get the Platforms and ExcludedPlatforms later + cfgPackage, ok := d.cfg.Root.GetPackage(pkg.Raw) + if !ok { + return nil, nil, fmt.Errorf("package %s not found in config", pkg.Raw) + } + + if err := d.removeFromConfig(pkg.Raw); err != nil { + return nil, nil, err + } + // Calling addToConfig with the original package names, since it will + // automatically append @latest if search is able to handle that. + // If not, it will fallback to the nixpkg format. + added, err := d.addToConfig(ctx, []string{pkg.Raw}, devopt.AddOpts{ + Platforms: cfgPackage.Platforms, + ExcludePlatforms: cfgPackage.ExcludedPlatforms, + }) + if err != nil { + return nil, nil, err + } + if slices.Contains(added.addedNames, pkg.Raw) { + // Re-added under the same (unversioned) name: resolve it fresh. + d.forgetLockEntry(pkg.Raw) + } + migrated = append(migrated, added) + } + + return pending, migrated, nil +} + func (d *Devbox) inputsToUpdate( opts devopt.UpdateOpts, ) ([]*devpkg.Package, error) {