Cache empty chunked responses - #13410
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a cache correctness gap where chunked responses with an empty body could fail to commit cache metadata (leading to later requests going back to origin), and adds replay-based coverage to prevent regressions. The change is implemented by ensuring cache writes create a VIO even for zero-byte bodies and by teaching the cache write path to treat an explicit zero-byte write as an empty document.
Changes:
- Start a zero-byte cache write VIO for empty-body cache writes so closing the cache VC commits (rather than aborts) the entry.
- Mark zero-byte cache writes as “empty document” in
CacheVCand ensure object size is set to 0 in that case. - Extend the negative-caching replay to cover empty chunked bodies for both negative (404) and successful (200) responses.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/gold_tests/cache/replay/negative-caching-300-second-timeout.replay.yaml | Adds replay coverage for caching empty chunked 404 and 200 responses (issue #11313). |
| src/proxy/http/HttpTunnel.cc | Ensures cache-write consumers get a VIO even when the computed write size is 0. |
| src/iocore/cache/CacheVC.cc | Treats do_io_write(..., nbytes=0, ...) as an empty-document write and propagates that into cache metadata handling. |
9790120 to
5a9d6fc
Compare
5a9d6fc to
e8ec617
Compare
e8ec617 to
0036ea4
Compare
0036ea4 to
8d9eb58
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
tests/gold_tests/cache/replay/negative-caching-300-second-timeout.replay.yaml:124
- This transaction asserts
proxy-request: expect: absent(origin should not be contacted), but it still specifies aserver-response. In the replay framework this can make the test inconsistent (it may try to drive/validate an origin exchange that must not happen). Fix by removing theserver-responseblock for this transaction, or, if you intend to verify behavior when the origin returns 502, dropexpect: absentand explicitly assert the origin request.
proxy-request:
expect: absent
server-response:
status: 502
reason: Bad Gateway
headers:
fields:
- [ Content-Length, 0 ]
src/iocore/cache/CacheVC.cc:223
- The same state update (
f.allow_empty_doc = 1plusalternate.object_size_set(0)) is duplicated in bothdo_io_writeanddo_io_close. To reduce the chance of these paths drifting over time, factor this into a small helper (e.g.,mark_empty_doc()) and call it from both places.
if (nbytes == 0) {
// A zero-byte write represents an empty document, while closing without a
// write represents a header-only update.
f.allow_empty_doc = 1;
if (alternate.valid()) {
alternate.object_size_set(0);
}
}
src/iocore/cache/CacheVC.cc:241
- The same state update (
f.allow_empty_doc = 1plusalternate.object_size_set(0)) is duplicated in bothdo_io_writeanddo_io_close. To reduce the chance of these paths drifting over time, factor this into a small helper (e.g.,mark_empty_doc()) and call it from both places.
if (alerrno == -1 && vio.op == VIO::WRITE && vio.get_reader() != nullptr && vio.nbytes == 0) {
// The write may have started with an unknown length and been finalized
// at zero after the response framing was parsed.
f.allow_empty_doc = 1;
if (alternate.valid()) {
alternate.object_size_set(0);
}
}
Empty chunked responses can complete without starting a cache write VIO, or can start with an unknown length that is later finalized at zero. ATS treats both as empty unwritten entries and sends later requests back to origin. AuTests can also cross a log-rolling boundary before checking custom logs, producing an unrelated intermittent failure. This patch starts zero-byte cache writes and recognizes successfully closed write VIOs whose final length is zero. This preserves the empty-document state while keeping header-only cache updates distinct. This also disables log rolling for stale-response log assertions and covers negative and successful empty responses. Fixes: apache#11313
8d9eb58 to
0c86bee
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
tests/gold_tests/cache/replay/negative-caching-300-second-timeout.replay.yaml:124
- This transaction asserts
proxy-request: expect: absent(no origin request), but still defines aserver-response. In replay tests, specifying aserver-responsewhile asserting no upstream request can be contradictory and may cause the harness to wait for or validate an origin exchange that should not occur. Prefer removing theserver-responseblock here (or changing it to an explicitexpect: absentequivalent if supported by the replay schema) so the transaction strictly verifies cache hit behavior.
proxy-request:
expect: absent
server-response:
status: 502
reason: Bad Gateway
headers:
fields:
- [ Content-Length, 0 ]
Empty chunked responses can complete without starting a cache write VIO, or can start with an unknown length that is later finalized at zero. ATS treats both as empty unwritten entries and sends later requests back to origin. AuTests can also cross a log-rolling boundary before checking custom logs, producing an unrelated intermittent failure. This patch starts zero-byte cache writes and recognizes successfully closed write VIOs whose final length is zero. This preserves the empty-document state while keeping header-only cache updates distinct. This also disables log rolling for stale-response log assertions and covers negative and successful empty responses. Fixes: apache#11313 (cherry picked from commit d0119c4)
|
Cherry-picked to the 10.2.x branch as e7a6e44 for the 10.2.0 release. |
Empty chunked responses never start a cache write VIO, so ATS aborts
the cache entry and sends later requests back to the origin.
This starts a zero-byte cache write and preserves the empty-document
state through cache setup. It also adds coverage for negative and
successful responses.
Fixes: #11313