Fix race condition in concurrent downloads for duplicate URLs

[ooye-sanket]

Fix race condition in concurrent downloads for duplicate URLs

Root Cause:
The race condition occurs because download_queue.rb creates separate download futures for each Downloadable object. In the docbook formula, the same URL (docbook-5.2.zip) appears both as the main formula download and as a resource. Even though they point to the same file, they create different Downloadable objects, so both attempt to download simultaneously and lock the same .incomplete file.

Solution Approach:
Instead of tracking downloads by Downloadable object (which treats the formula and resource as separate downloads), I've implemented deduplication based on cached_location (the actual file path). This ensures that when multiple downloadables need the same file, they share a single download operation.

Result:
The docbook formula and its xml52 resource both need docbook-5.2.zip, but now only one download is initiated and both share it. No more lock conflicts!

When HOMEBREW_DOWNLOAD_CONCURRENCY is enabled and a formula has the same URL listed both as the main download and as a resource (e.g., docbook), concurrent download attempts would try to lock the same .incomplete file, causing an "operation in progress" error.

The issue was that download_queue.rb created separate download futures for each Downloadable object, even when multiple downloadables shared the same cached_location (file path). This resulted in race conditions when downloading identical files.

This commit fixes the issue by:

• Adding @downloads_by_location hash to track downloads by file path
• Deduplicating downloads based on cached_location instead of Downloadable object identity
• Ensuring multiple downloadables sharing the same file reuse the same download future

When the same file is needed by multiple downloadables, they now share a single download operation, preventing concurrent lock attempts on the same .incomplete file.

Fixes #21425

• Have you followed the guidelines in our Contributing document?
• Have you checked to ensure there aren't other open Pull Requests for the same change?
• Have you added an explanation of what your changes do and why you'd like us to include them?
• Have you written new tests for your changes? Here's an example.
• Have you successfully run brew lgtm (style, typechecking and tests) with your changes locally?

[MikeMcQuaid]

Thanks @ooye-sanket, great work so far, some questions!

[MikeMcQuaid]

Thanks for clarifying! A few more questions ❤️

[ooye-sanket]

I think it might make more sense to make the output also "one per file" rather than "one per downloadable"? What do you think?

That makes sense 👍

Since the actual operation is already deduplicated to one download per file, showing output per file would be more accurate and simpler for users. Instead of rendering progress for each downloadable (formula + resource), we could render a single progress entry per cached_location.

I can refactor the fetch loop to iterate over @downloads_by_location directly and update the output to be “one per file” rather than “one per downloadable”, if that aligns with your preference. Happy to make that change.

[ooye-sanket]

Does @symlink_targets need cleared here?

Good catch! Yes, @symlink_targets should also be cleared. After the downloads complete and symlinks are created, we don't need to keep that data around. I'll add:

downloads.clear
@downloads_by_location.clear
@symlink_targets.clear

[ooye-sanket]

May be worth considering if it's worth having attr_reader for the instance variables?

For @downloads_by_location and @symlink_targets, they're internal implementation details, so I don't think they need attr_reader. They're only used within the class methods. The existing downloads method already provides access to the downloads hash when needed.

[ooye-sanket]

@MikeMcQuaid should i implement: refactoring to use downloads_by_location directly. This will:

• Remove the downloads hash
• Show one progress line per file (instead of per downloadable)
• Add @symlink_targets.clear to cleanup
• Result in cleaner, simpler code

if yes, then I'll push the changes shortly.

[MikeMcQuaid]

@ooye-sanket yes, this sounds great, thank you so much 😍

[ooye-sanket]

@MikeMcQuaid Refactored as discussed! Changes:

• Removed downloads hash completely
• Now using @downloads_by_location directly in fetch loop
• Output shows one line per file (e.g., "docbook-5.2.zip") instead of per downloadable
• Added cleanup for all tracking hashes
• Code is now simpler and more accurate to what's actually happening

[MikeMcQuaid]

Thanks @ooye-sanket! Code looks good. Only issue now from local testing is we want to retain the existing format of e.g.

==> Fetching downloads for: ffmpeg-full
✔︎ Bottle Manifest ffmpeg-full (8.0.1)                                                                                   Downloaded  108.1KB/108.1KB
✔︎ Bottle Manifest giflib (5.2.2)                                                                                        Downloaded   10.8KB/ 10.8KB
✔︎ Bottle giflib (5.2.2)                                                                                                 Downloaded  153.0KB/153.0KB
✔︎ Bottle Manifest highway (1.3.0)                                                                                       Downloaded    9.7KB/  9.7KB
✔︎ Bottle highway (1.3.0)                                                                                                Downloaded  896.7KB/896.7KB
✔︎ Bottle Manifest imath (3.2.2)                                                                                         Downloaded    7.4KB/  7.4KB
✔︎ Bottle imath (3.2.2)                                                                                                  Downloaded  194.3KB/194.3KB
✔︎ Bottle Manifest libdeflate (1.25)                                                                                     Downloaded    7.8KB/  7.8KB
✔︎ Bottle libdeflate (1.25)                                                                                              Downloaded  113.3KB/113.3KB
✔︎ Bottle Manifest openjph (0.26.0)                                                                                      Downloaded   12.2KB/ 12.2KB
✔︎ Bottle openjph (0.26.0)                                                                                               Downloaded  157.5KB/157.5KB
✔︎ Bottle Manifest openexr (3.4.4_1)                                                                                     Downloaded   15.6KB/ 15.6KB
✔︎ Bottle openexr (3.4.4_1)                               

rather than the new:

✔︎ cask.jws.json                                                                                                         Downloaded   15.3MB/ 15.3MB
✔︎ formula.jws.json                                                                                                      Downloaded   32.0MB/ 32.0MB
==> Fetching downloads for: ffmpeg-full
✔︎ 7332868699be1d72494b42c0966eca2044bb87ceb55952d79068bdff04542aa6--ffmpeg-full-8.0.1-1.bottle_manifest.json            Downloaded  108.1KB/108.1KB
✔︎ 283773c4d2db4fe867419d7eea6811a6417889d78fad8871041c07f49b22d2a1--giflib-5.2.2.bottle_manifest.json                   Downloaded   10.8KB/ 10.8KB
✔︎ 995b62b1518a0deae8dd9c0016ee099e69ae5a0e54bed3370983a80522d71783--giflib--5.2.2.arm64_tahoe.bottle.tar.gz             Downloaded  153.0KB/153.0KB
✔︎ 9c45ed9405a2a0988cb2c80afd415bcdacca07ef87ac47c277905cb96a0b611e--highway-1.3.0.bottle_manifest.json                  Downloaded    9.7KB/  9.7KB
✔︎ aa8ca7b1c25da4f0dd9d753cee28095bc01fbefcb01490047e20bc6e949a599a--highway--1.3.0.arm64_tahoe.bottle.tar.gz            Downloaded  896.7KB/896.7KB
✔︎ 82322ae05e389eb5b059000e61d0a7acb8176046888dc2e15344c8fd7704ade4--imath-3.2.2.bottle_manifest.json                    Downloaded    7.4KB/  7.4KB
✔︎ 74571d1c82f2668cb71807eb96f5fb3b7be738c2caf8dede9226d35b4a8b83b2--imath--3.2.2.arm64_tahoe.bottle.tar.gz              Downloaded  194.3KB/194.3KB

if it's easier to do that by continuing to use downloads as before: no worries, feel free to go back to that. Given you're adding downloadable_to_location here anyway I don't mind what 3 variables are used as long as we can fix the issue you've fixed here and preserve the (better) current output format. Users don't need to be told the actual filenames. If we're deduping: it's fine to just hide/lie in the output about something being downloaded for two formulae/resources/places.

Thanks again!

[ooye-sanket]

Thanks @ooye-sanket! Code looks good. Only issue now from local testing is we want to retain the existing format of e.g.

==> Fetching downloads for: ffmpeg-full
✔︎ Bottle Manifest ffmpeg-full (8.0.1)                                                                                   Downloaded  108.1KB/108.1KB
✔︎ Bottle Manifest giflib (5.2.2)                                                                                        Downloaded   10.8KB/ 10.8KB
✔︎ Bottle giflib (5.2.2)                                                                                                 Downloaded  153.0KB/153.0KB
✔︎ Bottle Manifest highway (1.3.0)                                                                                       Downloaded    9.7KB/  9.7KB
✔︎ Bottle highway (1.3.0)                                                                                                Downloaded  896.7KB/896.7KB
✔︎ Bottle Manifest imath (3.2.2)                                                                                         Downloaded    7.4KB/  7.4KB
✔︎ Bottle imath (3.2.2)                                                                                                  Downloaded  194.3KB/194.3KB
✔︎ Bottle Manifest libdeflate (1.25)                                                                                     Downloaded    7.8KB/  7.8KB
✔︎ Bottle libdeflate (1.25)                                                                                              Downloaded  113.3KB/113.3KB
✔︎ Bottle Manifest openjph (0.26.0)                                                                                      Downloaded   12.2KB/ 12.2KB
✔︎ Bottle openjph (0.26.0)                                                                                               Downloaded  157.5KB/157.5KB
✔︎ Bottle Manifest openexr (3.4.4_1)                                                                                     Downloaded   15.6KB/ 15.6KB
✔︎ Bottle openexr (3.4.4_1)                               

rather than the new:

✔︎ cask.jws.json                                                                                                         Downloaded   15.3MB/ 15.3MB
✔︎ formula.jws.json                                                                                                      Downloaded   32.0MB/ 32.0MB
==> Fetching downloads for: ffmpeg-full
✔︎ 7332868699be1d72494b42c0966eca2044bb87ceb55952d79068bdff04542aa6--ffmpeg-full-8.0.1-1.bottle_manifest.json            Downloaded  108.1KB/108.1KB
✔︎ 283773c4d2db4fe867419d7eea6811a6417889d78fad8871041c07f49b22d2a1--giflib-5.2.2.bottle_manifest.json                   Downloaded   10.8KB/ 10.8KB
✔︎ 995b62b1518a0deae8dd9c0016ee099e69ae5a0e54bed3370983a80522d71783--giflib--5.2.2.arm64_tahoe.bottle.tar.gz             Downloaded  153.0KB/153.0KB
✔︎ 9c45ed9405a2a0988cb2c80afd415bcdacca07ef87ac47c277905cb96a0b611e--highway-1.3.0.bottle_manifest.json                  Downloaded    9.7KB/  9.7KB
✔︎ aa8ca7b1c25da4f0dd9d753cee28095bc01fbefcb01490047e20bc6e949a599a--highway--1.3.0.arm64_tahoe.bottle.tar.gz            Downloaded  896.7KB/896.7KB
✔︎ 82322ae05e389eb5b059000e61d0a7acb8176046888dc2e15344c8fd7704ade4--imath-3.2.2.bottle_manifest.json                    Downloaded    7.4KB/  7.4KB
✔︎ 74571d1c82f2668cb71807eb96f5fb3b7be738c2caf8dede9226d35b4a8b83b2--imath--3.2.2.arm64_tahoe.bottle.tar.gz              Downloaded  194.3KB/194.3KB

if it's easier to do that by continuing to use downloads as before: no worries, feel free to go back to that. Given you're adding downloadable_to_location here anyway I don't mind what 3 variables are used as long as we can fix the issue you've fixed here and preserve the (better) current output format. Users don't need to be told the actual filenames. If we're deduping: it's fine to just hide/lie in the output about something being downloaded for two formulae/resources/places.

Thanks again!

Thanks for the detailed feedback that makes sense!
I’ll push an updated revision shortly.