DownloadLock: wait for another process's download instead of failing

[dduugg]

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).

When two separate brew processes need to download the same file at once (most commonly two brew bundle parallel-install workers that both need to fetch an undeclared implicit dependency, see #23342 for that specific scheduling gap), the loser currently dies immediately with Error: A `brew install ...` process has already locked ..., because LockFile#lock takes a non-blocking flock. The error message already tells the user to "wait for it to finish or terminate it to continue", so this makes that happen automatically instead of requiring a manual retry.

DownloadLock#lock_or_wait retries with a short sleep between attempts until the holder releases the lock, printing a "Waiting for another Homebrew process..." warning once rather than repeatedly. Only CurlDownloadStrategy#fetch (the actual download path) switches to it. FormulaLock/CaskLock and DownloadLock's existing usage in cleanup.rb (which deliberately wants to skip an in-progress download rather than wait for it) keep the current fail-fast behavior unchanged.

This is deliberately independent of #23342: it fixes the underlying race for any two processes contending on the same download, regardless of what causes them to collide, rather than trying to predict every possible cause of a collision ahead of time.

Step-by-step reproduction

Not easily reproducible on demand outside of #23328's own repro (two processes racing to download the same file), but the fix is covered by a real (non-mocked) end-to-end check using two live flocks across threads, in addition to unit tests: a locked DownloadLock genuinely blocks a second instance's lock_or_wait until released, then it succeeds.

---

• Have you followed our Contributing guidelines?
• Have you checked for other open Pull Requests for the same change?
• Have you explained what your changes do? Performance claims (e.g. "this is faster") must include Hyperfine benchmarks.
• Have you explained why you'd like these changes included, not just what they do?
• For bug fixes, have you given step-by-step brew commands to reproduce the bug?
• Have you written new tests (excluding integration tests)? Here's an example.
• Have you successfully run brew lgtm (style, typechecking and tests) locally?

---

• AI was used to generate or assist with generating this PR.

Used Claude Code to investigate #23328, design and implement this fix, and write the accompanying tests. Verified by running brew typecheck and brew style --changed (clean), running the full lock_file, download_strategies/curl, and cleanup spec suites (all passing, confirming the untouched fail-fast paths still work), and a manual real (non-mocked) end-to-end script using two threads and actual flock calls that confirmed a contended lock_or_wait genuinely blocks and then acquires the lock once the holder releases it, in the expected timeframe. Reviewed the full diff by hand before opening this PR.

---

[MikeMcQuaid]

Thanks, approach looks good, a few tweaks perhaps.

[dduugg]

I prototyped a blocking-flock alternative to this in #23356 and have closed it. The comparison and the measurements are in this comment rather than here, to keep this thread on this implementation. Short version: blocking flock wakes about 321x faster, that turns out to be imperceptible against waits measured in seconds to minutes, and making the flock long enough to block also makes it long enough to be interrupted, which admits an fd leak and an orphaned-lock case that a LOCK_NB flock cannot have.

Two things from that review apply to this PR:

1. The opoo will corrupt the parallel download display. HOMEBREW_DOWNLOAD_CONCURRENCY defaults to cores * 2, so concurrency above 1 is the normal case, and DownloadQueue#fetch drives a cursor-addressed redraw whose arithmetic assumes exactly one line per download. report_or_defer_failure exists specifically because an unscheduled write desyncs it, and a warning printed from a pool worker is exactly that. Separately, opoo is not gated on quiet while DownloadQueue sets quiet = concurrency > 1, so it prints even when the queue asked for silence. Routing it through report_or_defer_failure addresses both.

2. The give-up message should say how long it waited. As written the user is told to "wait for it to finish or terminate it to continue", which is what they just spent the timeout doing.

One test nit: allow(Time).to receive(:now).and_return(now, now + MAX + 1) is worth replacing with a small stubbed MAX_WAIT_SECONDS, because a global two-element Time.now sequence breaks as soon as anything else in the call path reads the clock.

Unrelated to either PR, lock_file_spec.rb deserves hardening. Mutation testing showed it passes with ignore_interrupts deleted from LockFile#lock entirely, and with the inode recheck deleted. I added coverage for the second in #23356 and can port it separately.

[MikeMcQuaid]

Thanks!

[Copilot]

Pull request overview

This PR improves Homebrew’s download locking behaviour so that when two brew processes contend for the same download lock, the losing process waits for the download to finish instead of failing immediately. This targets real-world parallelism (e.g. brew bundle parallel workers) while keeping fail-fast behaviour for other lock types and existing DownloadLock usages that intentionally skip.

Changes:

• Add DownloadLock#lock_or_wait, which retries lock acquisition with a single warning and a bounded wait.
• Extend OperationInProgressError to optionally report “gave up after waiting N seconds”.
• Switch CurlDownloadStrategy#fetch to use the new waiting lock behaviour and add/extend lock-related specs.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
Library/Homebrew/lock_file/download_lock.rb	Adds lock_or_wait with a max wait and one-time warning while retrying.
Library/Homebrew/download_strategy/curl_download_strategy.rb	Uses lock_or_wait during the actual download path.
Library/Homebrew/exceptions.rb	Updates OperationInProgressError messaging to optionally include waited time.
Library/Homebrew/lock_file.rb	Exposes locked_path for use in wait/warn messaging.
Library/Homebrew/test/lock_file/download_lock_spec.rb	New unit tests for lock_or_wait behaviour.
Library/Homebrew/test/lock_file_spec.rb	Adds tests covering LockFile#lock retry-on-disk-race and interrupt deferral.

Comments suppressed due to low confidence (2)

Library/Homebrew/test/lock_file/download_lock_spec.rb:66

• This test will emit an opoo warning to stderr (the first contended lock attempt) while also expecting an exception, which can make the spec suite noisy. Consider passing quiet: true here since the warning behaviour is tested separately.

      stub_const("DownloadLock::MAX_WAIT_SECONDS", 0)
      download_lock.lock
      allow(download_lock_copy).to receive(:sleep)

      expect { download_lock_copy.lock_or_wait }.to raise_error(OperationInProgressError)

Library/Homebrew/test/lock_file/download_lock_spec.rb:74

• This test will also print the initial opoo warning to stderr before raising, which can add noise to test output. Passing quiet: true keeps this focused on the exception message it is asserting.

      stub_const("DownloadLock::MAX_WAIT_SECONDS", 0)
      download_lock.lock
      allow(download_lock_copy).to receive(:sleep)

      expect { download_lock_copy.lock_or_wait }.to raise_error(/Gave up after waiting \d+ seconds/)

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[dduugg]

Following up on the two comments Copilot suppressed as low confidence, both of which asked for quiet: true in the give-up examples to cut stderr noise.

Both are applied, but the stated rationale does not hold for this repo. Library/Homebrew/test/spec_helper.rb reopens $stderr to File::NULL around every example unless it is :focused or HOMEBREW_VERBOSE_TESTS is set, so a spec here cannot add noise to the suite output. Confirmed by running the file and grepping both streams for the warning, which finds nothing with or without the change. quiet: true is still worth keeping in those two examples, just for a different reason: they assert on the raised exception, so narrowing them to that is clearer.

Checking it did turn up a real gap, though one I introduced rather than Copilot. My fix for the "warns only once" comment replaced output(/Waiting for another Homebrew process.../).to_stderr with receive(:opoo).once, which tightened the count but dropped the only assertion on the message content, so nothing would have caught a reworded or empty warning. That example now asserts both:

expect(download_lock_copy).to receive(:opoo).once.with(
  /Waiting for another Homebrew process to finish downloading/,
).and_call_original

Verified it bites: changing the warning text fails that example, where a moment earlier it passed.

[MikeMcQuaid]

Thanks!