DownloadLock: wait on a blocking flock instead of failing (alternative to #23343)

[dduugg]

What does this change do, and why?

Draft: this is an alternative to #23343, opened for comparison rather than to merge alongside it. Both fix the same half of #23328, so pick one and I'll close the other. Posting this because @MikeMcQuaid asked on #23343 whether we could use "signals or similar here and not just sleep".

It turns out no signals are needed. flock(LOCK_EX) without LOCK_NB is already a kernel-level blocking wait, so the kernel wakes us the instant the holder releases. The polling in #23343 only exists because LockFile#lock hardcodes LOCK_NB. So this version adds a blocking: option to LockFile#lock and moves the open/flock/inode-recheck sequence into a private acquired? helper.

Measured comparison against #23343

Wake latency is how long after the holder releases the waiter actually acquires. 6 trials each, with deliberately non-interval-aligned hold durations so polls are never accidentally synchronised with the release.

implementation	wake latency (mean)	wake latency (max)	CPU while waiting
polling, sleep 0.1 (#23343)	62.4ms	99.4ms	0.0038s
polling, sleep 1.0	946.0ms	987.1ms	0.0010s
blocking flock (this PR)	0.12ms	0.21ms	0.0020s

Blocking is roughly 500x more responsive than the 0.1s polling in #23343 and uses less CPU than it, so it removes the interval tradeoff I was weighing over there entirely.

The subtle part, and why the diff touches LockFile

A naive version of this introduces a Ctrl-C hang. LockFile#lock wraps its body in ignore_interrupts, and a blocking flock inside ignore_interrupts swallows SIGINT completely: the process stays blocked (printing "One sec, cleaning up...") until the lock frees, which could be an hour. I verified that experimentally before writing this. The polling approach in #23343 sidesteps it only by accident, because its sleep happens outside ignore_interrupts.

So here the blocking wait deliberately sits outside ignore_interrupts, while the non-blocking path keeps its existing wrapping. FormulaLock, CaskLock, and cleanup.rb's stale-download handling therefore keep their current behaviour byte for byte, and lock_file_spec.rb passes unchanged, which is the main evidence for that. Timeout.timeout caps the wait (it does interrupt a blocked flock, and File#flock has no timeout parameter of its own).

The cost of this approach versus #23343 is that it restructures a shared, correctness-sensitive primitive rather than only touching DownloadLock. That is the main thing worth weighing.

On using concurrent-ruby or another library instead

Worth ruling out explicitly, since we already depend on concurrent-ruby: it can't address this. Every primitive it offers (Event, Semaphore, CountDownLatch, ReadWriteLock, the atomics) is built on Ruby's Mutex/ConditionVariable and coordinates threads within one process. There is no flock usage anywhere in the gem. This race is between two separate brew processes, so shared-memory primitives can't see each other. Cross-process advisory locking is what flock is for, and it's already in stdlib.

Within a single process there's nothing to fix: DownloadQueue already dedupes by cached_location, so two of its threads never race on the same download. The contention is strictly cross-process.

Third-party gems in this space (filelock, lockfile, and friends) are thin wrappers over File#flock, so they'd add a vendored dependency (plus .licenses/ entries and RBIs) for no functional gain.

Step-by-step reproduction

Same as #23328 and #23343 (two processes racing on one download). Covered here by unit tests plus a real, non-mocked check using two live flocks: a held lock genuinely blocks a second instance until released, a real SIGINT interrupts a blocked wait immediately, and the wait cap raises rather than hanging forever.

---

• 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 prototype this alternative, run the comparison benchmarks, and write the tests. Verified by:

• Running brew typecheck and brew style --changed (clean).
• Running the lock_file, download_strategies/curl and cleanup suites (all passing). lock_file_spec.rb passes without modification, which is the check that the non-blocking path's semantics survived the restructure.
• Establishing the ignore_interrupts hazard experimentally rather than assuming it: a forked child blocked on flock, sent a real SIGINT, comparing bare / inside-ignore_interrupts / inside-Timeout.timeout. Only the ignore_interrupts case swallowed the signal and hung, which is what drove the design here.
• Re-running the same real-SIGINT check against the finished implementation (Interrupt raised immediately, ~0.002s CPU).
• Benchmarking wake latency and CPU for both approaches. My first attempt reported misleadingly low polling latency (~6ms) because the hold duration was an exact multiple of both poll intervals and had accidentally synchronised the polls with the release. The table above is from the corrected benchmark.
• Reviewing the full diff by hand before opening this PR.

---

[dduugg]

I reviewed this in depth before deciding between it and #23343, including reproducing the numbers in the description above. One real defect turned up, which I have fixed on the branch, and I still think #23343 is the one to land. Detail below so the reasoning is on the record rather than just the outcome.

A blocking flock admits a defect that a non-blocking one cannot

LockFile#acquired? opened the lock file and then called flock, with nothing closing it if an exception unwound out of that call. A blocking flock makes that reachable three ways: Timeout::Error at MAX_WAIT_SECONDS, a real Ctrl-C, and DownloadQueue#cancel raising Interrupt into a worker thread.

Two consequences, both confirmed empirically:

1. One leaked file descriptor per interrupted wait, in a class that already odies on Errno::EMFILE.
2. In the window after flock returns but before @lockfile is assigned, the process keeps the lock with nothing referencing it. unlock short-circuits on @lockfile.nil?, so CurlDownloadStrategy#fetch's ensure download_lock.unlock(unlink: true) neither releases nor unlinks. RetryableDownload then retries, and because flock is per-open-file-description the retry blocks for the full hour on a lock its own process holds. The error then tells the user that another brew process has locked the path, about itself.

The window is milliseconds rather than nanoseconds, because the three filesystem syscalls between the flock and the assignment are each interrupt checkpoints. A repro that aligns the holder's release with the timeout expiry hit it 20 times in 400 attempts.

Fixed by assigning or closing in an ensure, which takes it to 0 in 400 with no descriptor growth. That commit also adds tests for the interrupted-after-acquiring case, for the wait actually elapsing, and for the inode recheck, which nothing covered before.

The defect is gone, but it is still why I would not land this. It exists because the flock is long enough to be interrupted. #23343's flock is always LOCK_NB and returns immediately, its deadline check is synchronous, and its lock stays wholly inside ignore_interrupts, so it is structurally incapable of both failure modes.

The measured win holds up, and does not pay for the cost

The benchmark above is directionally right. I measured blocking wake latency at 0.164ms mean over 20 randomized trials against 52.5ms for 0.1s polling. The CPU claim is real and I understated it: paired and interleaved at n=20, blocking used 0.000326s against polling's 0.005182s, with polling higher in 20 of 20 trials and non-overlapping ranges. My table's 1.9x on n=6 invites dismissal as noise when the effect is robust.

Two corrections. The true ratio is about 321x rather than 500x, matching the ~310x that theory predicts. And both my 500x and my polling row are phase-locked to a single hold duration, so neither is a stable figure. "Two to three orders of magnitude" is what I should have written.

None of which changes that 50ms of average wake latency is invisible against a wait measured in seconds to minutes, and one wakeup per 100ms is not a real cost. A 321x improvement on an imperceptible quantity does not buy much, and here it is paid for by restructuring a primitive that serializes the Cellar, the Caskroom and the download cache, in a file that has already needed two race fixes (4bd75d4, 969005a).

My evidence for the restructure being safe was not evidence

I cited lock_file_spec.rb passing unchanged as the main proof that the non-blocking path survived. Mutation testing shows that suite lets through deleting ignore_interrupts outright, and deleting the inode recheck. I confirmed the second directly: replacing the guard with if false leaves every example in both spec files green.

The inode recheck is the most correctness-sensitive thing this diff moved, from raise OpenFileChangedOnDisk plus retry to return false plus a loop. The suite is blind to it, and always was, so this is a gap in the tests rather than a new problem. It does mean the claim I made for this PR rested on a test that cannot detect the change being made.

Three other corrections to the description above

• "Swallows SIGINT completely" is wrong. The signal is deferred, not swallowed. The handler runs within 0.42ms and Interrupt is raised 2.66ms after the holder releases. Every user-visible symptom I described is accurate, but the mechanism is not, and repeated Ctrl-C does not help either.
• "FormulaLock, CaskLock and cleanup.rb keep their current behaviour byte for byte" is too strong. path.dirname.mkpath and the @lockfile.present? early return both moved outside ignore_interrupts, and OperationInProgressError now originates one frame deeper. No functional regression, but not byte for byte.
• "the same half of Brew bundle races for locks on packages with overlapping dependencies #23328" should have said that bundle: serialize installs sharing an implicit dependency #23342 fixed the scheduling half, and this addresses the remaining generic two-process race.

One gap that applies to whichever lands

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 an opoo from a pool worker is exactly that. opoo is also 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 fixes both halves. I will raise this on #23343.

Also worth carrying across: waiting makes resume-after-crash work, because RetryableDownload preserves the .incomplete file for --continue-at, so a waiter that acquires after the holder dies resumes the partial download instead of failing. And the fix only covers CurlDownloadStrategy and its subclasses, since the VCS strategies take no download lock, though they never raised OperationInProgressError either.

Closing this in favour of #23343.

_{Review was AI-assisted, consistent with the disclosure in the description. Every claim above that is stated as measured was reproduced on this branch, and the two numbers I trusted without re-running my own repro (the orphaned lock rate, and the mutation survival) I verified independently before writing them down.}