bundle: fix rack lock conflicts and ordering in parallel installs - #23386
bundle: fix rack lock conflicts and ordering in parallel installs#23386dduugg wants to merge 6 commits into
Conversation
fd7e696 to
7abcde1
Compare
There was a problem hiding this comment.
Pull request overview
This pull request fixes brew bundle parallel-install scheduling so that entries which will contend on the same FormulaLock rack are not scheduled concurrently, including cases where the contended rack is the formula being installed (not just one of its dependencies). It also re-orients lock-conflict edges to match dependency-first ordering to avoid cycles that previously forced a silent fully-serial fallback.
Changes:
- Replace recursive-dependency-based conflict modelling with
lock_names(formula’s own rack + recursive dependency racks, normalised to rack names). - Order lock-conflict edges by a dependency-first topological order (cycle-tolerant) instead of Brewfile order.
- Add unit tests covering: dependency-free formula conflicts, edge orientation, cycle diagnostics, and lock-name normalisation/alias handling.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| Library/Homebrew/bundle/parallel_installer.rb | Builds lock-conflict sets including each formula’s own rack and orients conflict edges using a dependency-first topo order (with cycle diagnostics). |
| Library/Homebrew/bundle/brew.rb | Adds lock_names helper to return canonical rack names for a formula and its recursive dependencies (with unavailable-formula fallback). |
| Library/Homebrew/test/bundle/brew_spec.rb | Adds focused tests for Brew.lock_names normalisation and unavailable-formula fallback behaviour. |
| Library/Homebrew/test/bundle/installer_spec.rb | Updates scheduler specs to use lock_names and adds coverage for the newly fixed race/orientation/cycle cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
`FormulaInstaller#lock` locks the formula it is installing as well as all of its recursive dependencies, but `brew bundle`'s parallel scheduler decided which entries may run together by intersecting only the recursive dependency sets. A formula with no dependencies of its own, e.g. `xz`, therefore conflicted with nothing and was scheduled alongside entries whose installs lock `Cellar/xz`, so the loser died with `OperationInProgressError`. Compare the full set of racks each install locks instead. Racks are named after `Formula#name`, so resolve aliases and strip the tap prefixes that `Dependency#name` reports, otherwise a Brewfile `python` entry never matches the `python@3.14` that other formulae depend on. Including each entry's own name also lets a declared dependency edge and an order-based conflict edge point in opposite directions, e.g. for a Brewfile listing `python` before `xz`, which cycles and strands every entry in `run!`'s sequential fallback. Orient conflict edges by a dependency-first topological position rather than Brewfile position so that every edge in the graph points the same way.
Follow-up review fixes on top of the lock conflict change: - The cycle callback in Phase 5 discarded the detected cycle, so a Brewfile whose declared dependencies disagree degraded to a fully serial install with nothing to explain why. Name the cyclic entries with `odebug`, matching how `Brew.sort!` already reports the same class of cycle. - `entry_name_map` and `position` were inferred as `T.untyped`, which left `build_dependency_map`'s declared `T::Hash[String, T::Set[String]]` return unverified. Annotate both so the signature is actually checked. - Rename the Phase 7 block variable to `other`: the loop walks entries in Brewfile order and filters to earlier ones by topological position, so `earlier` described the filter rather than the iteration. - Cover the two behaviours the new tests did not distinguish: tap-prefixed `Dependency#name` values being reduced to rack names, and the implicit pioneer being chosen in dependency order rather than Brewfile order (the latter builds a cycle under the previous `entries.find`).
Phase 7 built its result with `each_with_object({})`, whose inferred type
is a bare shape, so `build_dependency_map`'s declared
`T::Hash[String, T::Set[String]]` return was satisfied vacuously.
Building it with `entries.to_h` types the result without a fourth
`T.let`. Injecting a non-`Set` value now fails `brew typecheck`, where
against `each_with_object` it passed.
Cover the two behaviours that were left unpinned:
- `tsort_with_cycles` in place of plain `tsort`, together with the
`odebug` that names the cyclic entries. A Brewfile listing `gh`
alongside one of `gh`'s own dependencies closes a loop, because
`HOMEBREW_VERIFY_ATTESTATIONS` makes `gh` a declared dependency of
every other entry, and plain `tsort` raises `TSort::Cyclic` there.
- the tap prefix strip in the `FormulaUnavailableError` fallback of
`lock_names`, which the existing example missed because it used a
name with no tap to strip.
Also give the implicit pioneer example a lock set that `lock_names`
could actually return: `alpha` declares a dependency on `beta`, so
`beta` is necessarily in `alpha`'s recursive set.
Follow-up to review of the parallel install scheduler. Four correctness gaps and the readability problem that hid them. `Brew.lock_names` resolved aliases for the entry itself but ran recursive dependencies through `Utils.name_from_full_name`, which strips tap prefixes without resolving aliases. `Dependency#name` is the string passed to `depends_on`, so `depends_on "python"` reported a `python` rack while the rack `FormulaInstaller#lock` takes is `python@3.14`: the two lock sets never intersected and the racing installs this PR set out to serialize still ran in parallel. Resolve each dependency through its formula instead. The method-level rescue also covered `recursive_dependencies`, so one unresolvable transitive dependency discarded every already-resolved rack name; it now falls back per-dependency and warns like `formulae_by_full_name` does. The attestation edge made every entry wait on the Brewfile's `gh`, including the entries `gh` itself depends on. That is a guaranteed cycle, and `run!` answers a cycle by installing the whole Brewfile serially, so `HOMEBREW_VERIFY_ATTESTATIONS` plus `brew "gh"` alongside any of its dependencies silently ignored `--jobs`. Skip the edge when `gh` already locks everything the entry locks, which is exactly when `gh` depends on it. Cask entries were compared on Brewfile cask tokens, never the formula racks a cask install locks, so a cask depending on formula `xz` still raced an entry that locks `xz`. Casks now report the racks `Cask::Installer` takes, built from the same dependency graph it resolves them with, and cask-on-cask ordering becomes a declared edge rather than a token-set intersection that could not fire. Cycle diagnostics were `odebug` while the consequence is a visible degradation, so use `opoo` with remediation advice as `Brew.sort!` does. `build_dependency_map` had grown to seven numbered phases in ~110 lines; split it into one helper per phase. `Brew::Topo` moves to `Utils::StringTopologicalHash` next to the graph it wraps, so the scheduler depends on the shared utility rather than reaching sideways into the formula dumper, and `sorted_names` recovers the node type that TSort's untyped interface discarded.
0a1a2ac to
39acadc
Compare
Review follow-up. Two of the previous commit's fixes were wrong and a third
was a no-op sold on a false premise.
The attestation guard asked whether `gh`'s rack closure covered an entry, but
the cycle it protects against lives in the declared-dependency graph, and
those are different graphs: lock closures come from `recursive_dependencies`
and include build dependencies, while ordering edges come from
`formula_dep_names`, which is runtime-only. `gh`'s sole dependency `go` is a
build dependency, so `formula_dep_names("gh")` is empty and no attestation
edge can close a cycle through it at all. The superset test fired anyway,
because `{gh, go}` covers `{go}`, and dropped the one edge that sequences
`gh`: `prepare_attestation_verification!` deliberately skips the upfront
bootstrap when `gh` is a Brewfile entry. For `brew "go"` plus `brew "gh"`
that inverted the two, leaving `go` to verify an attestation with no `gh`
installed. Decide on reachability in the graph the cycle would form in
instead, which also subsumes the cask special case: a cask is never in the
formula dependency graph, so it always keeps the edge.
`Cask.lock_names` collected the `Formula` nodes of
`graph_package_dependencies`, which prunes build and test dependencies, but
`FormulaInstaller#lock` locks the full `recursive_dependencies`. That dropped
exactly the dependencies this PR is about: the first commit's own example,
`xz` reaching `dust` through `rust` and `llvm`, is a build chain. `cask
"droid"` reported 2 racks against a ground truth of 28, so `brew "xz"` plus
`cask "droid"` still raced. Expand each formula through `Brew.lock_names`.
Resolving each dependency through `to_formula` in `Brew.lock_names` was a
no-op: `Dependency.expand` already canonicalises aliases and renames through
`dup_with_formula_name`, so `Formula["ipython"].recursive_dependencies`
already reports `python@3.14` for `depends_on "python"`. It only doubled
`Formulary.factory` calls, and the per-dependency rescue was unreachable
because an unresolvable transitive dependency makes the expansion raise
before it returns. Reverted to stripping the tap prefix. The split rescue
stays: it keeps the formula's own canonical rack when the tree cannot be
expanded, where the old single rescue fell back to the requested name, which
may be an alias that names no rack.
The cycle warning asked the user to report a Brewfile that declares a
dependency loop, which the DSL cannot express, and repeated remediation that
`Brew.sort!` already prints for the same stale keg tab data in the same run.
Adds the coverage those behaviours lacked: each of the cask rack expansion,
the cask attestation edge, the cask-on-cask ordering edge and the warning
text now fails when reverted, as does the `go`/`gh` ordering.
`implicit_pioneer` is inlined, `StringTopologicalHash` defines
`tsort_each_node` privately like its sibling instead of overriding
`each_key` behind a rubocop disable, and `merge_lock_conflicts` defaults its
lookups.
Two corrections to the previous commit message: the phase split is five
helpers rather than one per phase, because the name map, declared
dependencies and their resolution are one job, and it moves coupling to the
formula dumper up rather than down, since the scheduler now requires
`bundle/brew` and `bundle/cask` explicitly instead of inheriting them. Its
justification is type hygiene, not readability: eleven `T.let` before the
split, seven after, and the one it removed from `position` was an assertion
nothing checked.
MikeMcQuaid
left a comment
MikeMcQuaid
left a comment
There was a problem hiding this comment.
Thanks! A few general comments:
- some of the Ruby commends seem a little excessive
- I think the readability would be improved with less indirection e.g. fewer single-use functions and single-use classes not in the same file of their use
- I'm thinking this might be the last time we try this before we just rip out this parallel functionality
Addresses review feedback on comment volume and indirection. `build_dependency_map`'s five single-use helpers are inlined back into it, per the repo guideline to inline methods that are not reused twice or needed for unit tests. None were called more than once and no spec targeted them directly; the specs drive `build_dependency_map` itself, so coverage is unchanged. `Utils::StringTopologicalHash` is dropped and `Brew::Topo` restored exactly as it was, so `utils/topological_hash.rb` and its spec are untouched by this branch again. The two classes differed only in node type, `String` against `CaskOrFormula`, which is not enough to justify a second class sitting in a different file from either caller. Comments in the changed hunks are cut to the reasons that are not evident from the code. No behaviour change: every scheduling case this PR fixes produces the same dependency map as before, and the attestation cases still match `main` exactly.
dduugg
changed the title
|
Thanks, all three taken. Comments and indirection. Fixed in fcb8afa. The five single-use helpers I'd split out of Net effect on the diff: 613/184 across 9 files, down to 457/107 across 7. No behaviour change, every scheduling case still produces the same dependency map. I've also rewritten the description, which had drifted as the implementation changed, and removed my own narrative comments from the thread. On ripping out the parallel functionality. That's a fair call to make, and I'd rather give you the honest input than argue for my own patch. The case against keeping it is in this PR's own "Notes for reviewers": I fixed four ways the scheduler mismodelled what a child process locks, and I found at least three more while doing it, including duplicate Brewfile entries still racing from a two-line Brewfile. The pattern is that the scheduler maintains a second, approximate model of Homebrew's locking, and that model is wrong in a new way every time someone looks closely. The one thing that does catch this class is a differential against what the installer actually locks, rather than a unit test of the model. That is how three of these four were found. It is maintainable if you want to keep the feature, but it is also the admission: the scheduler needs continuous policing against the real implementation, because nothing in the model itself tells you when it has drifted. There's also a floor on the payoff. So if you'd rather remove it, I think that's defensible and I'm happy to open that PR instead. If you'd rather keep it for now, this PR does fix the reported bug and two of its siblings, and I'd suggest the duplicate-entry case as the next one to close since it reproduces from two lines. Either way it's your call, not mine, and I won't be offended if the answer is to delete it. |
Note we expect to remove this 🔜ish.
This seems suboptimal. Instead can we not ensure we ensure all these deps if needed first so they don't conflict later? |
Good to know, and it means I was leaning on a temporary detail. I've dropped it from the argument in the description. It turns out the problem is much less Linux-specific than I made it sound, which your second point gets at directly.
You're right that it's suboptimal, and worse than I described. It isn't really about Measured on a 15-entry Brewfile of common formulae, macOS, no sandbox involvement:
That last row is your suggestion, and it does work, but not quite as "ensure the deps first" alone. Pre-installing them changes nothing by itself, because The obvious way to reach it isn't available, though. That's a Happy to take any of the three: narrow the lock, remove parallel install, or leave this PR as the bug fix it is. |
Yeh, I think this makes more sense to do. Really if we have a "read-only lock" on a rack/keg: we shouldn't care if anything/anywhere else also has that lot. Feels like we want a semaphore here rather than the current mutex? I also wonder if part of the issue here is we now have two layers of parallelism:
If so: maybe it'd be enough for |
What does this change do, and why?
Fixes one of the causes of the still-reproducing report on #23328 (#23328 (comment)). Further causes are listed under Notes for reviewers and are not fixed here. #23342 and #23343 fixed
DownloadLockcollisions. This one is aFormulaLock, which those PRs deliberately left fail-fast, so neither applies:/…/Cellar/xzis aFormulaLock(FormulaLock.new(rack_name)locksHOMEBREW_CELLAR/rack_name), and the advice line ("Please wait for it to finish…" rather than "Gave up after waiting N seconds…") pins the raise to plainLockFile#lock, notDownloadLock#lock_or_wait.ParallelInstaller#build_dependency_mapdecides which Brewfile entries may install concurrently. Its model of what a child process locks was incomplete in three ways, and its ordering edges pointed two different directions. Each is fixed here.1. An entry's own rack was missing from its lock set.
FormulaInstaller#locklocks the formula it is installing and all of its recursive dependencies. The scheduler intersected only the recursive dependency sets.xzhas no dependencies of its own, so its set was empty, it intersected nothing and it got zero ordering edges, while almost every other entry hasxzin its recursive set and locksCellar/xz.2. Cask entries modelled no rack locks at all.
Cask::Installerruns aFormulaInstallerfor each missingdepends_on formula:dependency, and those take the same rack locks abrewentry would. Casks were compared on Brewfile cask tokens instead, a different namespace, so a cask never conflicted with a formula. Casks now report the racks their formula dependencies lock, expanded through the sameBrew.lock_namesa formula entry uses. This matters becauseTopologicalHash.graph_package_dependenciesprunes build and test dependencies whileFormulaInstaller#lockdoes not, so collecting that graph's formulae alone still under-reports:cask "droid"reaches 28 racks, not the 2 its direct dependencies suggest.3. Cask-on-cask ordering could never fire. It was expressed as an intersection of Brewfile cask tokens, which for
cask "a"depending oncask "b"gives{"b"}against{}and never intersects. It is now a declared dependency edge via a newCask.cask_dependencies.4. Conflict edges were oriented by Brewfile order, not dependency order. Declared-dependency edges follow the dependency graph, while the pairwise conflict rule always made the later Brewfile entry wait on the earlier one. A Brewfile listing a dependent before its dependency therefore cycled whenever that dependency has recursive dependencies of its own,
ready_entrieswas empty on the first iteration andrun!silently installed everything serially, ignoring--jobs.brew "wget"beforebrew "openssl@3"does this on unpatchedmainwith no involvement from the rack change; 7 of 25 randomly generated four-entry Brewfiles drawn from common core formulae hit the fallback. A leaf dependency such asxzescaped it, because the back-edge needed a non-empty recursive-dependency intersection. Conflict edges are now oriented by dependency-first topological position, so every edge points the same way and the graph is acyclic whenever the declared dependencies are.A cycle in the declared dependencies is still tolerated rather than fatal.
tsort_with_cycleskeeps the rest of the order usable, and the cyclic entries are now named withopoorather than only under--debug, since the consequence is a visible loss of parallelism.Brew.sort!already explains the usual cause, stale keg tab dependency data, so that advice is not repeated.Two supporting points, neither a behaviour change. The attestation edge that makes entries wait on a Brewfile
ghis now added after the declared-dependency graph is built and skipped for entriesghitself reaches, so it cannot close a cycle.gh's only dependency is a build dependency, so it appears in no ordering edge and the resulting order is identical tomainon every Brewfile I tried; the guard matters ifghever gains a runtime dependency that is also a Brewfile entry. Separately,build_dependency_mapkeepsmain's shape rather than being split into helpers, andBrew::Topostays where it is, after review feedback on indirection.Step-by-step reproduction
All three reproduce on macOS with no container. The first is the reporter's own
Brewfile.minimalfrom #23328:$ printf 'brew "bat"\nbrew "xz"\n' > /tmp/Brewfile && brew bundle install --file=/tmp/Brewfile --jobs 4batreachesxzthroughrustthenllvm, so its child process locksCellar/xz, butxzhas no dependencies of its own and so gets no ordering edge.main{"bat" => [], "xz" => []}[["bat", "xz"]]{"bat" => [], "xz" => ["bat"]}[["bat"], ["xz"]]The cask case needs a cask whose formula dependencies reach a Brewfile formula:
$ printf 'brew "xz"\ncask "superslicer"\n' > /tmp/Brewfile && brew bundle install --file=/tmp/Brewfile --jobs 4superslicerdeclareszstd, which reachesxz.main[["xz", "superslicer"]][["xz"], ["superslicer"]]The edge orientation case needs a Brewfile listing a dependent before its dependency:
$ printf 'brew "wget"\nbrew "openssl@3"\n' > /tmp/Brewfile && brew bundle install --file=/tmp/Brewfile --jobs 4main{"wget" => ["openssl@3"], "openssl@3" => ["wget"]}{"wget" => ["openssl@3"], "openssl@3" => []}[["openssl@3"]], [["wget"]]Notes for reviewers
More places where the scheduler's model of what a child process locks is still incomplete. These are separate causes, so they are not touched here and I am happy to open follow-ups:
brew "xz"twice collapses to one key whilerun!keeps two distinct entries and schedules them together. Still reproduces on this branch. Needs entry deduplication inBundle::Installer.Upgrade.upgrade_dependentsreinstalls dependents, so it locks reverse dependencies.FormulaInstaller#linkcallsUnlink.unlink_link_overwrite_formulae, which takes aKeg#lockon every linkedlink_overwriteformula, andlink_overwriteis not a dependency relation.Keg#lockalso locks the oldname rack of whatever it locks, which the scheduler never models either.Formula[]can raiseTapFormulaAmbiguityError,FormulaSpecificationErrororUntrustedTapError, none of which descend fromFormulaUnavailableError, so they escape therescueand abort the wholebrew bundle installbefore anything is installed. Pre-existing and unchanged by this PR.The bigger limit on how parallel this can ever be, which no amount of scheduling fixes:
FormulaInstaller#locklocks the formula's entire recursive dependency closure, with no check for whether a dependency is already installed. Two entries therefore contend whenever they share any transitive dependency. On a 15-entry Brewfile of common formulae on macOS, with no sandbox involvement at all, 63 of the 105 pairs share a rack, which is 10 batches at--jobs 8.On Linux without a system
bwrapthe implicitbubblewrapdependency makes that total rather than partial, though that dependency is expected to go away soon.The scheduler can only serialize correctly here, not avoid the contention, because the contention is in what the child process locks. Removing it means not locking dependencies that are already installed and not being modified: the
unless ignore_deps?guard inFormulaInstaller#lockalready skips the closure, and with entries' own racks alone the same Brewfile has 0 of 105 conflicting pairs. Reaching that through--ignore-dependenciesis not an option, sincebrew installcalls it "an unsupported Homebrew developer option", so it would mean narrowinglockitself, or making a dependency lock shared rather than exclusive. Either is aformula_installerchange rather than abundleone and belongs in its own PR.There is a larger question about whether this scheduler should exist at all, raised in review and worth stating here.
brew bundleshells out to onebrew installper entry, and each child builds its ownDownloadQueuesized atHOMEBREW_DOWNLOAD_CONCURRENCY, which defaults to twice the CPU count. So--jobs 4on a 12-core machine is 4 processes of 24 download threads each. Meanwhile a singlebrew install X Y Zalready downloads and pours in parallel through one shared queue, and becauseFormulaInstaller.lockedis class-level with an early return, it takes rack locks exactly once and cannot contend with itself. Passing the whole Brewfile to onebrew installwould therefore delete this entire class of bug rather than model around it. It is not a drop-in replacement, sincebundlevariesargs,--force,--overwriteand--skip-linkper entry, splits formulae from casks, and choosesinstallagainstupgradeper entry, so it would need grouping by action and flags plus per-entry failure attribution. This PR fixes the bug that is live today; it is not an argument against that being the better long-term shape.brewcommands to reproduce the bug?brew lgtm(style, typechecking and tests) locally?AI was used. Claude Code (Claude Opus) was used to investigate the report on #23328, identify which lock was actually contended, implement the change and write the tests, then to re-review the result across several passes. Verification, all of which I ran and read myself:
brew lgtmclean (typecheck,style --changed,tests --changed), plus the spec files of callers outside the changed-file mapping, which--changeddoes not select.--only=file:linetarget, by reverting the corresponding production change and confirming the failure was the expected one, including the cask rack expansion, the cask attestation edge, cask-on-cask ordering and the cycle warning text.build_dependency_mapagainst the real dependency graph atmainand at this branch, then replayingrun!'s batching loop over the result. Not against mocks.recursive_dependencieskeeps:builddependencies at every level (which is whyxzis indust's set at all, viarustthenllvm), thatgraph_package_dependenciesdoes not, and thatFormulaInstaller#lockkeys onFormula#namerather than the requested name.x86_64Linux in a Debian 13 container built from the reporter's Dockerfile, running the unpatched commit and the patched one back to back over the reporter's 19-entry Brewfile, including reproducing the reporteddust/xzcollision on unpatchedmain.One correction worth stating, since it bears on how much weight to give the rest. A mid-PR commit claimed
Brew.lock_namesunder-reported racks for aliased dependencies and resolved each dependency throughto_formula. That was wrong:Dependency.expandalready canonicalises aliases and renames viadup_with_formula_name, so the change was a no-op. The probe that appeared to justify it built aDependencyby hand, which never passes throughexpand. It was reverted after an element-by-element comparison across all 8,527 core formulae and 156,807 dependency edges found zero differing results.brew auditalso rejects a bare alias dependency outright, so no core formula could exhibit the case regardless. I reviewed the full diff by hand before pushing.