download_strategy: fix caching of :latest downloads#20137
Conversation
305167e to
5ff58d2
Compare
MikeMcQuaid
left a comment
MikeMcQuaid
left a comment
There was a problem hiding this comment.
Thanks again @EricFromCanada!
|
Appreciate the quick fix @EricFromCanada . |
|
@EricFromCanada, this is actually a breaking change for my scenario. We have a proxy (Cloudflare) in front of a private host for internal bottles. If the user's authentication has lapsed, the proxy will redirect to a login page that serves HTML whose To increase the determinism of our devenv, we seed Homebrew's downloads cache bottles we know engineers will need. We wouldn't want a redirect to Cloudflare's login page to invalidate this cache. Notably, the login page does not serve a I've made a change to our private tap that bypasses this logic, so I'm not blocked; but I wanted to describe a use-case where the cache invalidation logic might not quite be dialed in yet. |
|
Thanks for the background info. Seems logical; I'll attempt to cook up a test case. |
|
@boblail What's the (anonymized) output of |
Here's what I ran $ curl -IL https://{HOST}/{BOTTLE_PATH}
HTTP/2 302
date: Mon, 30 Jun 2025 18:24:13 GMT
content-type: text/html
content-length: 143
location: https://{TENANT}.cloudflareaccess.com/cdn-cgi/access/login/{HOST}?{ARGS}
set-cookie: {COOKIE}
access-control-allow-credentials: true
cache-control: private, max-age=0, no-store, no-cache, must-revalidate, post-check=0, pre-check=0
expires: Thu, 01 Jan 1970 00:00:01 GMT
set-cookie: {COOKIE}
server: cloudflare
cf-ray: 957fb3065beab9f8-SEA
HTTP/2 200
date: Mon, 30 Jun 2025 18:24:14 GMT
content-type: text/html
content-length: 30070
set-cookie: {COOKIE}
strict-transport-security: max-age=31536000; includeSubDomains
cf-access-domain: {HOST}
cf-version: 20-f198bbf
content-security-policy: frame-ancestors 'none'; connect-src 'self' http://127.0.0.1:*; default-src https: 'unsafe-inline'
referrer-policy: strict-origin-when-cross-origin
x-content-type-options: nosniff
x-frame-options: DENY
x-xss-protection: 1; mode=block
server: cloudflare
cf-ray: 957fb306e969fa3b-SEA |
|
@EricFromCanada, yep! That fix works for my scenario 👍 Thanks! I can remove the monkey patch I referenced earlier:
|
4 participants
First, the
cached_location_valid = false if v.is_a?(Cask::DSL::Version) && v.latest?line was never being run, becauseversionis one of (NilClass, String, Version), not Cask::DSL::Version. It's intended to ensure that:latestcasks are always re-downloaded on each upgrade, but with what we have now we can do better than that.Further down is the cached location invalidation logic, which compares the cached file's modification date and file size against the results from a HEAD request on the download URL and invalidates if either are different. An earlier PR made this logic never run if the final download URL was a redirect. For anything with the version number in the URL this isn't usually a problem, but for resources behind a public URL that stays static and redirects to a new secondary URL on each update, they end up never being updated, e.g.
:latestcasks hosted on GitHub or GCloud or AWS.Currently the above change doesn't appear to be necessary (although this may not have always been the case). For example, this is the redirect that occurs for downloading
chromium:Note how the first request has a small content-length value. With some
odebugadded todownload_strategy.rb, we see that this isn't considered when evaluating the upstream file size.With an older nightly build of
wezterm@nightlycached, we currently get:After making these changes:
Fixes #20084. If there's still cases of cached locations being invalidated when they shouldn't be, please comment below. @boblail