Repository navigation
fix: serve a stale response on a failed revalidation only when allowed - #179
Draft
beanow-at-crabnebula wants to merge 9 commits into
Draft
beanow-at-crabnebula wants to merge 9 commits into
beanow-at-crabnebula wants to merge 9 commits into
Conversation
Adds tests that revalidate a stale 200 and get a full response back, for both the buffered and the streaming cache. ### Why Revalidation only handles 200 and 304 today. A 200 that can't be stored, or any other status, leaves the old entry in place. A cacheable 404 or 410 isn't stored either. These tests fail on the first case, a 200 with `no-store`. Assisted-by: claude-opus-5-5
…tion `conditional_fetch` now handles every status other than 304 like it handled a 200. When the response can be cached, it replaces the stored one. When it can't, the stored one is removed. A server error keeps the stored response, as if the server hadn't responded. ### Why RFC 9111 s4.3.3 says a full response to a validation request is used to satisfy the request and can replace the stored response. Before, a 200 that couldn't be stored, or any status besides 200 and 304, left the old entry in place. That entry would still be served in `ForceCache`, `OnlyIfCached` and `IgnoreRules` modes, and when the next revalidation fails. Cacheable statuses like 404 and 410 were also never stored on revalidation. ### Testing The new tests in `http-cache-reqwest` pass for the buffered and streaming caches. The test suites of the core, reqwest, tower, tower-server, surf and ureq crates pass. Assisted-by: claude-opus-5-5
Adds 204 cases to the full response revalidation tests: with `max-age`, without caching headers, and with `no-store`. ### Why The Tauri updater uses a cacheable 204 to say "you're up to date". It should replace a stored 200 that announced an update. Before the fix, the stored 200 stayed in the cache. Assisted-by: claude-opus-5-5
The full response revalidation tests now check the stored body, not only its status. When the new response is fresh, a third request must be served from the cache. ### Why Review feedback: the tests checked the returned body and the stored status, which left the stored body and its reuse unchecked. ### Testing Marking the stale 204 case as fresh makes both tests fail, so the reuse check catches a response that isn't served from the cache. Assisted-by: claude-opus-5-5
When revalidating gets a server error, the stale stored response is now served only when it does NOT have `must-revalidate`. With `must-revalidate`, the 5xx is passed on. Both the buffered and the streaming cache had the check inverted. ### Why RFC 9111 s4.3.3 lets a cache that gets a 5xx while validating act as if the server hadn't responded, and send the stored response, subject to the constraints of s4.2.4. `must-revalidate` (s5.2.2.2) is what forbids reusing a stale response without successful validation. The network error path below already got this right. ### Testing `revalidation_500` in `http-cache-reqwest` now covers both cases, for the buffered and the streaming cache. Both cases fail without the fix. The `revalidation_500` tests in the tower, surf and ureq crates store a response without `must-revalidate`, and still expect the stale response. The test suites of the core, reqwest (including the middlewest backend), tower, tower-server, surf and ureq crates pass. Assisted-by: claude-opus-5-5
…ix/revalidation-5xx-must-revalidate
…sed on The full response revalidation tests now store the 200 with `max-age=0, must-revalidate`. ### Why With the 5xx fix, a server error without `must-revalidate` serves the stale response. The 503 case checks that a server error passed on keeps the stored response, which now only happens with `must-revalidate`. Assisted-by: claude-opus-5-5
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #179 +/- ##
==========================================
+ Coverage 85.87% 91.09% +5.22%
==========================================
Files 19 19
Lines 7502 12042 +4540
==========================================
+ Hits 6442 10970 +4528
- Misses 1060 1072 +12 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…ates Their `revalidation_500` tests now also check that with `must-revalidate`, a 500 during revalidation is passed on, without a `Warning` header. Tower's test loops over both cases. Surf and ureq get a separate test. ### Why The 5xx fix changed these tests to store a response without `must-revalidate`. That left no test in these crates for a 5xx that reaches the client, which goes through each crate's own response handling. ### Testing Without the 5xx fix, the new cases fail in all three crates. Assisted-by: claude-opus-5-5
beanow-at-crabnebula
changed the title
must-revalidate
On a 5xx or a network error during revalidation, the stored response is now served stale only when it is the variant for this request, and has none of `no-cache` or `must-revalidate`. In a shared cache, the default, `proxy-revalidate` and `s-maxage` also prevent it. Otherwise the 5xx or the error is passed on. ### Why The fallback only checked `must-revalidate`. It served a stored response that `Vary` doesn't match, which can be another user's response with `Vary: Authorization`. And it served responses whose directives require successful validation before reuse (RFC 9111 s4.2.4, s5.2.2). ### Testing The reqwest 5xx tests cover each directive and a `Vary` mismatch, buffered and streaming. Each new case fails without the fix. A unit test covers the directive check, including `shared: false`. Assisted-by: claude-opus-5-5
beanow-at-crabnebula
changed the title
must-revalidateThis branch has not been deployed
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.
Depends on #177
Github doesn't let me create a Stack when both base and head branch are on my fork.
When revalidation fails with a 5xx or a network error, the stored response is now served stale only when the spec allows it. Otherwise the error is passed on.
must-revalidate(5xx)no-cacheproxy-revalidate,s-maxage*Varymismatch)*= In a shared cache, the default.
Why
RFC 9111 s4.3.3 allows a cache to act as if the server hadn't responded when it gets a 5xx. Serving a stale response is then limited by s4.2.4:
no-cache,must-revalidate, and in a shared cacheproxy-revalidateands-maxage, require successful validation before reuse (s5.2.2).The old code only checked
must-revalidate, and the 5xx path had that check inverted. It also ignored aVarymismatch. WithVary: Authorization, one user could get another user's stored response whenever the origin returned a 5xx.Testing
Varymismatch, buffered and streaming. Each new case fails without the fix.must-revalidatecase.Assisted-by: claude-opus-5-5