Subtract the Age header when computing a cache deadline - #538
Merged
Conversation
ericmj
force-pushed
the
http-util-cache-age
branch
2 times, most recently
from
August 4, 2026 16:04
85aceed to
0b1959b
Compare
ericmj
marked this pull request as ready for review
August 4, 2026 16:06
maennchen
reviewed
Aug 4, 2026
`headers_to_cache_deadline/2` read only `max-age`, so a document a shared cache had already held for most of its stated lifetime was treated as fully fresh. Google serves discovery through a CDN, and the suite's exact 3_600_000 assertions became a range because of it. `Age` is subtracted from a lifetime the server stated, and only from that. The caller's `fallback_expiry` is a refresh interval it chose rather than a claim by the server, so an intermediary's `Age` says nothing about it and does not reduce it. `max-age=0` still lands on the fallback, as issue erlef#370 needs. Nothing left of a stated lifetime is reported as `0`, which widens the return type from `pos_integer()` to `non_neg_integer()`. That is not a usable refresh interval, so the loaders hold the result above a new `minimum_refresh` option, itself bounded by `fallback_expiry` so a caller asking for a short interval still gets it. Without that floor a nearly-stale response schedules an immediate reload, which for the provider configuration worker is a sustained request loop. `no-cache` and `no-store` are left alone. Honouring them means choosing a refresh interval for a library that has to cache to function, and that decision wants its own discussion.
ericmj
force-pushed
the
http-util-cache-age
branch
from
August 4, 2026 20:44
0b1959b to
5316682
Compare
`mix test.coverage` and `rebar3 coveralls send` are sibling jobs with the
same `needs`, and both downloaded every artifact in the run. The first
uploads its `cover` directory as `coverage-report`, which holds the same
three `.coverdata` files the second unpacks, so once that upload lands
`mv artifacts/*/*.coverdata` gets each basename twice and fails:
mv: will not overwrite just-created
'_build/test/cover/ct-OTP-27.3.4.coverdata' with
'artifacts/ct-coverage-27.3.4/ct-OTP-27.3.4.coverdata'
Whether it fires depends on which job downloads first, so it is
intermittent on a first run and certain on a re-run, where the previous
attempt's `coverage-report` is always present.
Both jobs use only the coverdata, so restrict the download to the
artifacts that carry it.
maennchen
approved these changes
Aug 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
headers_to_cache_deadline/2computes freshness frommax-agealone. RFC 7234 §4.2defines it as
max-ageminus theAgeheader, andAgeis what a shared cache reportsabout how long it has already been holding the response. Dropping that term means a
document a CDN has held for most of its lifetime is treated as brand new.
That's measurable against a live provider. Two assertions in
oidcc_provider_configuration_SUITEhard-code3_600_000for Google's discovery expiry;with the
Agesubtraction they come back around1_751_000, because Google served thatresponse with roughly 1849 seconds of age. So oidcc reported a document with 30 minutes
of freshness left as having a full hour. As
Ageapproachesmax-agethe reportedlifetime approaches double the truth. Those two assertions become range checks here,
since the exact number now depends on how long the CDN had the response.
It matters most to
oidcc_provider_configuration_worker, which driveserlang:send_after/3off this value and so schedules its refresh late by whatever the
Agewas. Callers thatpersist the derived timestamp rather than holding it in one process carry the error for
as long as they cache it.
The subtraction alone isn't safe, which is the other half of this change. Before it, the
only route to a small deadline was a provider sending a small
max-age, and exactly zerowas impossible because
clamp_expiry/2sends non-positive values to the fallback. Afterit, any response a shared cache is serving near the end of its lifetime produces one, and
zero is reachable. The worker hands that straight to
erlang:send_after/3and the backoffonly fires on
{error, _}, so it's an immediate refetch with nothing damping it: 6 requestsin 3 seconds against 2. So
oidcc_provider_configurationfloors the deadline withmax(Deadline, min(?MINIMUM_REFRESH, DefaultExpiry)), one minute. The innerminkeeps acaller that deliberately asked for a shorter
fallback_expiryfrom having it overruled.Flooring there rather than in the worker is what keeps
Expiry :: pos_integer()true inthe loaders' specs instead of widening to admit zero.
no-cacheandno-storeare deliberately left alone. Honouring them means returning zero,which is issue #370: kanidm sends
no-store, no-cache, max-age=0on discovery, and commit31c7f3dfixed the resulting crash by falling back to the caller's expiry. Reversing thathere would reintroduce it, and an RP refetching discovery on every request is not what a
provider misconfiguring its cache headers should get.
headers_to_cache_deadline/2's return widens frompos_integer()tonon_neg_integer(),which is the one part of this that isn't strictly additive. It's the only export in
oidcc_http_utilwithout-doc false, so it's hexdocs-visible by omission rather than byintent, and both in-tree callers go through
oidcc_provider_configuration, which floorsthe value before it reaches anyone. Say the word if you'd rather it were hidden.