bundle: serialize installs sharing an implicit dependency - #23342
Conversation
872f300 to
b4f7f8d
Compare
MikeMcQuaid
left a comment
There was a problem hiding this comment.
Thanks! Approach makes sense, just think it's possible to get this information another way instead.
b4f7f8d to
f053ab1
Compare
f053ab1 to
6adb00a
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates brew bundle鈥檚 parallel scheduler to account for Linux-only implicit formula dependencies (e.g. bubblewrap, gcc, glibc) that can be injected at formula instantiation time, so entries that would contend on the same implicit dependency are scheduled in separate batches to avoid lock/download races.
Changes:
- Add
DependencyCollector#implicit_dependency_namesto surface currently-applicable implicit dependency formula names. - Incorporate implicit dependency names into
brew bundle鈥檚 recursive dependency sets used for overlap/lock-conflict scheduling. - Add unit tests covering
implicit_dependency_names(generic + Linux) and the bundle scheduler鈥檚 serialization behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| Library/Homebrew/dependency_collector.rb | Introduces implicit_dependency_names helper to report currently-applicable implicit deps. |
| Library/Homebrew/bundle/parallel_installer.rb | Adds implicit deps into recursive dependency sets for lock-conflict scheduling in brew bundle. |
| Library/Homebrew/test/dependency_collector_spec.rb | Verifies default (non-Linux) implicit_dependency_names is empty. |
| Library/Homebrew/test/os/linux/dependency_collector_spec.rb | Adds Linux-specific tests for bubblewrap/gcc/glibc inclusion in implicit_dependency_names. |
| Library/Homebrew/test/bundle/installer_spec.rb | Adds a scheduler test ensuring entries are serialized when sharing an implicit dep. |
馃挕 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
brew bundle's parallel scheduler avoids running two entries in the same batch when their recursive declared dependencies overlap, but implicit dependencies added at formula-instantiation time (e.g. bubblewrap and its dependents on Linux, when the sandbox executable isn't installed yet) aren't declared on any formula, so two otherwise-unrelated formulae can both silently need to fetch the same bottle at once. Since DownloadLock's flock is non-blocking, the loser dies instead of waiting. Add DependencyCollector#implicit_dependency_names (empty by default, overridden on Linux to report bubblewrap/gcc/glibc when they'd currently be added), and fold it into every entry's recursive dependency set before computing overlaps, so entries needing the same implicit dependency are serialized like any other shared dependency. See #23328
6adb00a to
f959477
Compare
MikeMcQuaid
left a comment
There was a problem hiding this comment.
Yeh, this approach makes more sense I think! Thanks!
Two `brew` processes downloading the same file raced on the download lock and one failed outright with `OperationInProgressError`, which is half of #23328 (the scheduling half was fixed in #23342). Waiting is almost always what the user wants, since the holder is about to produce exactly the file this process needs. `DownloadLock#lock_or_wait` polls the existing non-blocking `flock` every 0.1s instead of giving up on the first failure. At 0.1s the wait costs about 0.15% of one core per waiting download, dominated by syscall overhead rather than real work, so the interval buys responsiveness for no meaningful CPU. The wait is capped at 3 minutes: an hour is always going to be too long, and giving up beats waiting much longer because `RetryableDownload` preserves the `.incomplete` file, so a retry resumes the holder's partial download via `--continue-at`. The warning is suppressed when the caller is already rendering progress. `HOMEBREW_DOWNLOAD_CONCURRENCY` defaults to `cores * 2`, and above 1 `DownloadQueue#fetch` drives a cursor-addressed redraw whose arithmetic assumes one line per download, so an unscheduled write from a pool worker desyncs it. `OperationInProgressError` also takes an optional `waited:` now, because telling someone to "wait for it to finish or terminate it to continue" after three minutes of waiting is not useful. The message is unchanged for every existing caller. Mutation testing found `lock_file_spec.rb` passed with the inode/unlink recheck neutered and with `ignore_interrupts` removed from `#lock`, so cover both. Deferring the interrupt itself can't be asserted in-process, since RSpec owns the `INT` handler that `ignore_interrupts` traps and `Thread#raise` bypasses `trap`, so the wrapper's presence is asserted instead.
Two `brew` processes downloading the same file raced on the download lock and one failed outright with `OperationInProgressError`, which is half of #23328 (the scheduling half was fixed in #23342). Waiting is almost always what the user wants, since the holder is about to produce exactly the file this process needs. `DownloadLock#lock_or_wait` polls the existing non-blocking `flock` every 0.1s instead of giving up on the first failure. At 0.1s the wait costs about 0.15% of one core per waiting download, dominated by syscall overhead rather than real work, so the interval buys responsiveness for no meaningful CPU. The wait is capped at 3 minutes: an hour is always going to be too long, and giving up beats waiting much longer because `RetryableDownload` preserves the `.incomplete` file, so a retry resumes the holder's partial download via `--continue-at`. The warning is suppressed when the caller is already rendering progress. `HOMEBREW_DOWNLOAD_CONCURRENCY` defaults to `cores * 2`, and above 1 `DownloadQueue#fetch` drives a cursor-addressed redraw whose arithmetic assumes one line per download, so an unscheduled write from a pool worker desyncs it. `OperationInProgressError` also takes an optional `waited:` now, because telling someone to "wait for it to finish or terminate it to continue" after three minutes of waiting is not useful. The message is unchanged for every existing caller. Mutation testing found `lock_file_spec.rb` passed with the inode/unlink recheck neutered and with `ignore_interrupts` removed from `#lock`, so cover both. Deferring the interrupt itself can't be asserted in-process, since RSpec owns the `INT` handler that `ignore_interrupts` traps and `Thread#raise` bypasses `trap`, so the wrapper's presence is asserted instead.
Two `brew` processes downloading the same file raced on the download lock and one failed outright with `OperationInProgressError`, which is half of #23328 (the scheduling half was fixed in #23342). Waiting is almost always what the user wants, since the holder is about to produce exactly the file this process needs. `DownloadLock#lock_or_wait` polls the existing non-blocking `flock` every 0.1s instead of giving up on the first failure. At 0.1s the wait costs about 0.15% of one core per waiting download, dominated by syscall overhead rather than real work, so the interval buys responsiveness for no meaningful CPU. The wait is capped at 3 minutes, or at the caller's remaining `timeout:` budget when that is shorter, so waiting on the lock can't blow a deadline the caller asked for. An hour is always going to be too long, and giving up beats waiting much longer because `RetryableDownload` preserves the `.incomplete` file, so a retry resumes the holder's partial download via `--continue-at`. The warning is suppressed when the caller is already rendering progress. `HOMEBREW_DOWNLOAD_CONCURRENCY` defaults to `cores * 2`, and above 1 `DownloadQueue#fetch` drives a cursor-addressed redraw whose arithmetic assumes one line per download, so an unscheduled write from a pool worker desyncs it. `OperationInProgressError` also takes an optional `waited:` now, because telling someone to "wait for it to finish or terminate it to continue" after three minutes of waiting is not useful. The message is unchanged for every existing caller. Mutation testing found `lock_file_spec.rb` passed with the inode/unlink recheck neutered and with `ignore_interrupts` removed from `#lock`, so cover both. Deferring the interrupt itself can't be asserted in-process, since RSpec owns the `INT` handler that `ignore_interrupts` traps and `Thread#raise` bypasses `trap`, so the wrapper's presence is asserted instead.
Two `brew` processes downloading the same file raced on the download lock and one failed outright with `OperationInProgressError`, which is half of #23328 (the scheduling half was fixed in #23342). Waiting is almost always what the user wants, since the holder is about to produce exactly the file this process needs. `DownloadLock#lock_or_wait` polls the existing non-blocking `flock` every 0.1s instead of giving up on the first failure. At 0.1s the wait costs about 0.15% of one core per waiting download, dominated by syscall overhead rather than real work, so the interval buys responsiveness for no meaningful CPU. The wait is capped at 3 minutes, or at the caller's remaining `timeout:` budget when that is shorter, so waiting on the lock can't blow a deadline the caller asked for. An hour is always going to be too long, and giving up beats waiting much longer because `RetryableDownload` preserves the `.incomplete` file, so a retry resumes the holder's partial download via `--continue-at`. The warning is suppressed when the caller is already rendering progress. `HOMEBREW_DOWNLOAD_CONCURRENCY` defaults to `cores * 2`, and above 1 `DownloadQueue#fetch` drives a cursor-addressed redraw whose arithmetic assumes one line per download, so an unscheduled write from a pool worker desyncs it. `OperationInProgressError` also takes an optional `waited:` now, because telling someone to "wait for it to finish or terminate it to continue" after three minutes of waiting is not useful. The message is unchanged for every existing caller. Mutation testing found `lock_file_spec.rb` passed with the inode/unlink recheck neutered and with `ignore_interrupts` removed from `#lock`, so cover both. Deferring the interrupt itself can't be asserted in-process, since RSpec owns the `INT` handler that `ignore_interrupts` traps and `Thread#raise` bypasses `trap`, so the wrapper's presence is asserted instead.
What does this change do, and why?
Addresses one of two independent causes behind #23328 (per @MikeMcQuaid's request for a separate PR per cause).
brew bundle's parallel scheduler (bundle/parallel_installer.rb'sbuild_dependency_map) avoids running two entries in the same batch when their recursive declared dependencies overlap, matching the lockingFormulaInstaller#lockdoes. On Linux, though, formulae can silently gain an implicit dependency at instantiation time (bubblewrap, andgcc/glibcwhen build formulae are needed) whenever the relevant executable isn't installed yet, added viaFormula#add_global_deps_to_specrather than declared on any formula. Two entries with no declared dependency in common (e.g.batandxz) can therefore both silently need to fetch the same bottle at once. SinceDownloadLock'sflockis non-blocking, the loser used to die immediately withOperationInProgressErrorinstead of waiting.This adds
DependencyCollector#implicit_dependency_names, empty by default and overridden on Linux to report whichever of those names would currently be added, and folds it into every entry's recursive dependency set before computing overlaps, so entries that would both need the same implicit dependency get serialized like any other shared dependency.I wasn't able to fully verify the Linux-specific code path end to end on my own machine (macOS doesn't load
extend/os/linux/*at all, confirmed while investigating), so the Linux-specific unit tests here will get their first real execution on Linux CI. Opening as a draft for that reason, please let me know if you'd like anything adjusted before it's ready for a full review.Step-by-step reproduction
Given in #23328:
brew bundle installwith a Brewfile containing two formulae that share no declared dependency but would both pull inbubblewrapon a fresh Linux machine without it installed (e.g.batandxz).brewcommands to reproduce the bug?brew lgtm(style, typechecking and tests) locally?Used Claude Code to investigate #23328, trace the scheduler's overlap-detection logic and the implicit-dependency mechanism it misses, implement the fix, and write the accompanying tests. Verified by running
brew typecheckandbrew style --changed(clean), running the full spec files touched (dependency_collector_spec.rb,bundle/installer_spec.rb) and the fullbundledirectory (all passing), and manually confirming via a scratch script thatFormula#recursive_dependencies(no block) preserves:implicit-tagged dependencies rather than pruning them, which the scheduler's overlap check relies on. Reviewed the full diff by hand before opening this PR.