Correct quality precedence and cache-hit header consistency - #70
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be75795210
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| self.quality | ||
| .or_else(|| self.save.format_quality.get(format).copied()) | ||
| .or(self.default_quality) |
There was a problem hiding this comment.
Invalidate cache entries after changing quality precedence
When upgrading an installation that uses the recoverable disk or hybrid cache, a request containing format_quality that was previously encoded under IMGFORGE_QUALITY keeps the same processed_cache_key, so process_path returns the old bytes before this new precedence is evaluated. Such URLs therefore continue receiving the incorrectly configured quality until eviction or manual cache deletion; include a cache namespace/version that distinguishes outputs produced with the corrected precedence.
Useful? React with 👍 / 👎.
be75795 to
bcfb7a1
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bcfb7a10c3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let cached_source = SourceMetadata { | ||
| url: Some(decoded_url.clone()), | ||
| ..SourceMetadata::default() |
There was a problem hiding this comment.
Key cached provenance by the resolved source URL
When a disk or hybrid cache survives a restart that changes IMGFORGE_BASE_URL, a relative source path resolves to a new URL but processed_cache_key still depends only on the request path. A cache hit can therefore return bytes fetched from the old base URL while this newly constructed metadata emits a canonical Link for the new URL. Include the resolved source in the cache identity or persist the original source URL with the cached value so the header cannot misidentify cached content.
Useful? React with 👍 / 👎.
bcfb7a1 to
53a3ec9
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
53a3ec9 to
9bc3f76
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
9bc3f76 to
c68658d
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
c68658d to
e7bf091
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7bf091a1a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // build makes the option-string parser reject the whole encode, so a | ||
| // deployment linked against an older system libvips would fail every | ||
| // request that set preserve_hdr rather than merely losing the gain map. | ||
| if options.preserve_hdr.unwrap_or(false) && supports_gainmap_flag() { |
There was a problem hiding this comment.
Document the old-libvips HDR fallback
When running against libvips older than 8.16, this now returns successfully while omitting the gain map, but doc/5_processing_options.md:251 still tells users that enabling preserve_hdr makes encoding fail, and ROADMAP.md:24-27 still lists this runtime check as unimplemented. Update those documents so operators do not mistake a successful response for one that retained all requested HDR data.
Useful? React with 👍 / 👎.
e7bf091 to
49d0c29
Compare
49d0c29 to
013146c
Compare
013146c to
9fde2a0
Compare
9fde2a0 to
aa7a157
Compare
aa7a157 to
e341964
Compare
e341964 to
4d1339d
Compare
4d1339d to
4cbc8ef
Compare
59dcfdb to
4a4a7ce
Compare
4a4a7ce to
c811234
Compare
888c597 to
71708e6
Compare
Three things a review pass turned up. A configured IMGFORGE_QUALITY was seeding the URL's own quality field, which made it outrank a format_quality the URL had asked for. The configured value is now kept separately and consulted last, matching imgproxy: URL quality, then URL format_quality, then configuration, then the built-in default. The source URL is now resolved before the cache lookup rather than after. A source removed from IMGFORGE_ALLOWED_SOURCES was still being served out of the cache, and the canonical header appeared on a miss but not on a hit. preserve_hdr named the gainmap keep flag unconditionally. That flag only exists from libvips 8.16, and naming it on an older build makes the option-string parser reject the whole encode — so a deployment linked against an older system libvips failed every request that set the option instead of merely losing the gain map.
71708e6 to
a374194
Compare
Stacked on #69. Three things a review pass turned up.
A configured
IMGFORGE_QUALITYwas seeding the URL's own quality field, which made it outrank aformat_qualitythe URL had asked for. The configured value is now kept separately and consulted last, matching imgproxy: URLquality, then URLformat_quality, then configuration, then the built-in default.The source URL is now resolved before the cache lookup rather than after. A source removed from
IMGFORGE_ALLOWED_SOURCESwas still being served out of the cache, and the canonical header appeared on a miss but not on a hit.preserve_hdrnamed thegainmapkeep flag unconditionally. That flag only exists from libvips 8.16, and naming it on an older build makes the option-string parser reject the whole encode — so a deployment linked against an older system libvips failed every request that set the option instead of merely losing the gain map. The version is now checked once at startup.Validation
Plus a real image build, and a container serving every output format, Accept negotiation, the caching headers, and an animated GIF resize.