Bring the remaining docs up to date - #71
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 673f55eafc
ℹ️ 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".
| - **Key derivation**: the cache key is the full request path — processing options, `cachebuster`, and output format included. Any difference in the path is a different entry. Three things outside the path join it, because each changes the bytes without changing the URL: `IMGFORGE_DEFAULT_FORMAT` for URLs that name no format, the format chosen by content negotiation, and the effective `max_result_dimension`. That last one means lowering the ceiling retires entries stored under a higher one rather than serving an image the limit now forbids. | ||
| - **Population**: rendered bytes are inserted after a successful response. A failed write is logged and does not affect the response. | ||
| - **Invalidation**: there is no explicit purge. Caches are capacity-limited and evict least-recently-used entries; change the `cachebuster` token to force a miss when an upstream asset changes. | ||
| - **What a hit cannot reproduce**: `Last-Modified` comes from the source response, which a hit never makes, so it is absent on cache hits. `Cache-Control`, `ETag`, and the canonical `Link` are all derived from the configuration or the bytes and are identical either way. |
There was a problem hiding this comment.
When IMGFORGE_CACHE_CONTROL_PASSTHROUGH is enabled and the origin supplies Cache-Control, this header is not identical on cache hits: the miss path builds SourceMetadata from the fetched response, while the hit path in src/service/mod.rs constructs metadata without cache_control, causing response::cache_control to fall back to IMGFORGE_TTL or omit the header. Document Cache-Control alongside Last-Modified as origin data that a hit cannot reproduce, with the configured TTL as the possible fallback.
Useful? React with 👍 / 👎.
673f55e to
bbbda43
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bbbda43679
ℹ️ 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".
| ## How caching works | ||
|
|
||
| - **Key derivation**: the cache key is the full request path — processing options, `cachebuster`, and output format included. Any difference in the path is a different entry. | ||
| - **Key derivation**: the cache key is the full request path — processing options, `cachebuster`, and output format included. Any difference in the path is a different entry. Three things outside the path join it, because each changes the bytes without changing the URL: `IMGFORGE_DEFAULT_FORMAT` for URLs that name no format, the format chosen by content negotiation, and the effective `max_result_dimension`. That last one means lowering the ceiling retires entries stored under a higher one rather than serving an image the limit now forbids. |
There was a problem hiding this comment.
When IMGFORGE_ENABLE_CLIENT_HINTS is enabled, identical paths requested with different Width or DPR values produce distinct hint=... cache keys in processed_cache_key; therefore the statement that only three inputs outside the path join the key is incomplete. This omission can mislead operators diagnosing extra entries or invalidation behavior, so document the client-hint dimensions as another cache-key component.
Useful? React with 👍 / 👎.
|
|
||
| | Response | Cause | | ||
| | ------------------------- | --------------------------------------------------------------------------- | | ||
| | `304` | The request's `If-None-Match` matched the response's `ETag`. Requires `IMGFORGE_USE_ETAG`. | |
There was a problem hiding this comment.
A 304 does not require IMGFORGE_USE_ETAG: when IMGFORGE_LAST_MODIFIED_ENABLED is enabled and ETags are disabled, image_response compares If-Modified-Since with the delivered Last-Modified value and also returns 304 Not Modified. The current row incorrectly tells users that this status is available only through If-None-Match and ETags.
Useful? React with 👍 / 👎.
| On success the bytes are inserted into the cache; a failed write is logged but does not affect the response. imgforge replies `200 OK` with the encoded bytes, the matching `Content-Type`, and an `X-Request-ID` header for log correlation. | ||
|
|
||
| ## 8. Metrics & logging | ||
| The delivery headers are attached here — `Cache-Control`, `ETag`, `Last-Modified`, the canonical `Link`, `Vary: Accept`, and the CORS origin, each when configured. A request whose `If-None-Match` matches the `ETag` gets `304 Not Modified` with no body; the bytes were produced either way, so the saving is bandwidth rather than work. |
There was a problem hiding this comment.
When client hints are enabled, the response varies on Sec-CH-Width, Width, Sec-CH-DPR, and DPR, and Accept may be absent if format negotiation is disabled. Describing the delivery header specifically as Vary: Accept is therefore inaccurate for that configuration and obscures the dimensions shared caches must distinguish.
Useful? React with 👍 / 👎.
bbbda43 to
3b03c16
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b03c16ca9
ℹ️ 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".
| ├─ 7. populate cache │ | ||
| ├─ 6. fetch source ─── upstream status ──▶ 400 │ | ||
| │ too large / wrong MIME ──▶ 400 │ | ||
| ├─ 7. transform ────── over IMGFORGE_TIMEOUT ──▶ 408 │ |
There was a problem hiding this comment.
Move this timeout branch out of the transform-only stage: TimeoutLayer wraps the entire router in src/server.rs, so when IMGFORGE_TIMEOUT is shorter than an origin fetch, cache lookup, watermark fetch, or semaphore wait, the request can return 408 before transformation begins. The current diagram misdirects operators diagnosing a 408 as necessarily caused by image processing.
Useful? React with 👍 / 👎.
| ├─ 6. transform ────── over IMGFORGE_TIMEOUT ──▶ 408 │ | ||
| ├─ 7. populate cache │ | ||
| ├─ 6. fetch source ─── upstream status ──▶ 400 │ | ||
| │ too large / wrong MIME ──▶ 400 │ |
There was a problem hiding this comment.
For an ordinary processed request, IMGFORGE_ALLOWED_MIME_TYPES and the resolution constraint are enforced inside the blocking transformation closure, after open_source and after acquiring the worker permit; only raw/pass-through responses check them before returning. Labeling a wrong MIME as a stage-6 fetch failure, reinforced by line 51's stale reference to “the start of stage 6,” gives an incorrect account of when decoding and worker capacity have already been consumed.
Useful? React with 👍 / 👎.
| | `max_animation_frames` | A ceiling is in force. | | ||
| | `max_animation_frame_resolution` | A ceiling is in force. | | ||
|
|
||
| The three ceilings behave the same way: lowering one retires the entries stored under a higher one rather than serving an image the limit now forbids. A key is left untouched when its input does not apply, so turning any of these on does not invalidate a cache that never used it. |
There was a problem hiding this comment.
Enabling one of these ceilings does invalidate the previously usable namespace: processed_cache_key prepends mrd=..., maf=..., or mafr=... whenever the corresponding effective limit becomes Some, so an entry written while the limit was unset will no longer be found. This sentence tells operators the opposite of the behavior they will observe after enabling a limit.
Useful? React with 👍 / 👎.
| - **`preserve_hdr` on libvips below 8.16** — the `gainmap` keep flag does not exist there. imgforge checks the | ||
| runtime version once and drops the flag on an older build, so the request loses the gain map instead of failing; | ||
| the high-bit-depth half of the option still works. The published image ships 8.16.1 and is unaffected. |
There was a problem hiding this comment.
Update the processing-option catalog together with this corrected behavior: doc/5_processing_options.md still says that enabling preserve_hdr on libvips below 8.16 makes encoding fail, while this text and supports_gainmap_flag() show that only the gain map is dropped. Users consulting the primary option reference therefore still receive the obsolete and contradictory compatibility warning.
Useful? React with 👍 / 👎.
| │ source not allowed ──▶ 400 | ||
| │ | ||
| ├─ 3. cache lookup ─── hit ────────────────────────────┐ | ||
| ├─ 5. cache lookup ─── hit ────────────────────────────┐ |
There was a problem hiding this comment.
Changing cache lookup to stage 5 leaves doc/7_caching.md line 3 pointing readers to stage 3, which is now option parsing. Update that cross-reference as part of this renumbering so the two linked lifecycle descriptions agree.
Useful? React with 👍 / 👎.
3b03c16 to
2bbafd3
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2bbafd325c
ℹ️ 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".
| `IMGFORGE_MAX_SRC_FILE_SIZE` (or a per-request override) is enforced while the body streams, so an oversized source is abandoned mid-download — that one really does belong to the fetch. | ||
|
|
||
| ## 5. Option parsing | ||
| `IMGFORGE_ALLOWED_MIME_TYPES` and `IMGFORGE_MAX_SRC_RESOLUTION` are checked in stage 7, not stage 6: the resolution check needs the dimensions, so libvips has opened the buffer and a worker slot has already been taken by the time either runs. Treat them as limits on what gets *processed*, not as a barrier in front of the decoder. The exception is `raw` and a matching `skip_processing`, which never enter stage 7 at all — those two run both checks before returning the source bytes, so opting out of processing is not a way around them. |
There was a problem hiding this comment.
When a raw or matching skip_processing response was cached before IMGFORGE_ALLOWED_MIME_TYPES or IMGFORGE_MAX_SRC_RESOLUTION was tightened, process_path returns the cached bytes at src/service/mod.rs:189-208 without reaching enforce_source_constraints at lines 241-246; neither setting participates in processed_cache_key. Thus these checks do not always run before source bytes are returned, and operators relying on the new limits must purge/change the cache key or wait for eviction.
Useful? React with 👍 / 👎.
2bbafd3 to
ca41746
Compare
ca41746 to
0662093
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06620936c0
ℹ️ 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".
|
|
||
| | Input | Joins the key when | | ||
| | ----- | ------------------ | | ||
| | Output version | Always. Bumped by any release that changes the bytes an unchanged URL produces, so an upgrade retires entries rather than serving the old output indefinitely. | |
There was a problem hiding this comment.
When a cached request uses raw, processed_cache_key returns from the is_raw branch before applying OUTPUT_VERSION, so those entries survive an upgrade; only source-limit prefixes can retire them. Saying the output version joins “Always” and that an upgrade retires entries can therefore mislead operators troubleshooting persistent raw cache entries; document the raw exception.
Useful? React with 👍 / 👎.
0662093 to
8a2ab16
Compare
8a2ab16 to
e46fe1b
Compare
e46fe1b to
f97e34d
Compare
f97e34d to
c12dce3
Compare
4c7b48c to
a352b26
Compare
a352b26 to
915c754
Compare
915c754 to
d5ba690
Compare
d5ba690 to
d4be9ce
Compare
d4be9ce to
c37e4cf
Compare
c37e4cf to
19ea6e3
Compare
19ea6e3 to
c6efeeb
Compare
Request lifecycle gains the negotiation and delivery stages and the two new response codes; caching explains what joins the key besides the path, what a cache hit cannot reproduce, and how the client-side headers relate to the server-side cache.
The delivery layer now records the origin's Cache-Control and Last-Modified with each cache entry and serves them on hits, so the paragraph describing them as unavailable there described behaviour that no longer exists. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
c6efeeb to
3e1af5b
Compare
1 participant
Stacked on #70. Documentation only.
304, and404for an expired URL). The stage numbering was renumbered to match the diagram.Last-Modified, which comes from the source response), and how the client-side headers relate to the server-side cache.Cross-checked every
IMGFORGE_*variable in the docs againstsrc/constants.rs: no setting is undocumented, and nothing documented is absent from the code.Validation