Skip to content

Honor the RamCache copy flag in LRU and S3-FIFO - #13380

Draft
phongn wants to merge 1 commit into
apache:masterfrom
phongn:fix-ramcache-copy-honor
Draft

Honor the RamCache copy flag in LRU and S3-FIFO#13380
phongn wants to merge 1 commit into
apache:masterfrom
phongn:fix-ramcache-copy-honor

Conversation

@phongn

@phongn phongn commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

Summary

put(..., copy = true) is the caller's contract that buffers may not be shared between the cache and the caller in either direction. CacheVC requests it whenever RAM cache compression is configured (http_copy_hdr, CacheVC.cc), because it unmarshals HTTP headers in place in its own buffer after the put, and again on every RAM-cache hit.

The LRU and S3-FIFO RAM caches ignored the flag and shared buffers both ways. With proxy.config.cache.ram_cache.compress set and algorithm = 1 (the default) or 2:

  1. put stores the caller's buffer, and CacheVC's subsequent in-place unmarshal silently mutates the cached bytes;
  2. the next HTTP RAM hit hands that buffer back and unmarshals it again — HTTPInfo::unmarshal fails on the already-unmarshalled header (ink_assert in debug builds; broken HTTP RAM hits in release).

The http_copy_hdr gate keys off the compress config only, never the algorithm, so this is reachable purely by configuration today. This correctness fix stands alone (compression support for LRU is proposed separately) and is a backport candidate.

Changes

  • The copy contract is implemented once, as RamCache::copy_data_in() / RamCache::copy_data_out() on the base class; CLFUS's three pre-existing open-coded copies of the same logic are replaced with calls to them (no behavior change).
  • RamCacheLRU and RamCacheS3FIFO now honor copy the way CLFUS always has: copy on put, copy on get, and a copy = true put refreshes a resident entry that may still be sharing a caller's buffer from before a configuration change.
  • copy = false behavior is unchanged: zero-copy sharing, pinned by a test.
  • New Catch2 test (test_RamCacheCopy) pins the contract for all three policies: caller mutation after put, caller mutation of the returned buffer, the resident-refresh transition, and copy=false sharing. Before the fix, the three copy cases fail for LRU and S3-FIFO; CLFUS passes.

Notes for reviewers

  • The copied-in buffer is an exact-size xmalloc allocation, so block_size() recovers the data length and neither entry struct needs a new length field.
  • S3-FIFO's ghost-readmit and stale-auxkey paths go through the single insert site, so they inherit the copy handling.
  • The test keeps constructed caches reachable rather than destroying them: the policies have no destructors (entries are pool-allocated), so destroying a cache object would strand its entries under leak checkers.

CacheVC requests copy semantics (put copy=true) whenever RAM cache
compression is configured, because it unmarshals HTTP headers in place
in its own buffer after the put and again on every RAM hit. LRU and
S3-FIFO ignored the flag and shared buffers with the caller in both
directions, so with proxy.config.cache.ram_cache.compress enabled and
algorithm=1 (the default) or 2, the cached bytes were mutated after
insertion and every subsequent HTTP RAM hit re-unmarshalled an
already-unmarshalled header (assert in debug builds, header unmarshal
failure in release). Copy entries now copy on put, copy on get, and a
copy=true put refreshes a resident entry that may still be sharing a
caller's buffer, matching CLFUS.

This is a backport candidate for all release branches.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@phongn
phongn force-pushed the fix-ramcache-copy-honor branch from 514d888 to 1b53b1e Compare July 14, 2026 16:24
@phongn

phongn commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator Author

Tracking issue: #13381

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a correctness bug in the RAM cache implementations (LRU and S3-FIFO) by honoring the put(..., copy=true) contract, ensuring the cache never shares buffers with callers for copy entries (copy-on-put and copy-on-get). This prevents caller-side in-place mutations (e.g., CacheVC HTTP header unmarshal when RAM cache compression is enabled) from corrupting cached content and breaking subsequent RAM hits.

Changes:

  • Centralizes copy semantics into RamCache::copy_data_in() / RamCache::copy_data_out() and reuses them from CLFUS (no behavior change there).
  • Updates LRU and S3-FIFO to track per-entry copy state, copy on put/get when requested, and “refresh” existing resident entries when a later copy=true put occurs.
  • Adds a Catch2 unit test covering copy=true isolation in both directions, the copy=false sharing behavior, and the resident refresh transition across all three RAM cache policies.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/iocore/cache/unit_tests/test_RamCacheCopy.cc New unit test that pins the copy contract across LRU/CLFUS/S3-FIFO and covers regression scenarios.
src/iocore/cache/RamCacheS3FIFO.cc Implements copy handling on put/get and resident refresh for S3-FIFO entries.
src/iocore/cache/RamCacheLRU.cc Implements copy handling on put/get and resident refresh for LRU entries.
src/iocore/cache/RamCacheCLFUS.cc Replaces open-coded copy logic with shared base-class helpers (behavior-preserving refactor).
src/iocore/cache/P_RamCache.h Adds shared copy_data_in() / copy_data_out() helpers on the RamCache base class.
src/iocore/cache/CMakeLists.txt Registers the new RamCacheCopy Catch2 unit test.

@moonchen

Copy link
Copy Markdown
Contributor

Read through this and ran the tests locally. The bug reproduces as described: http_copy_hdr at CacheVC.cc:403 is computed from cache_config_ram_cache_compress and the doc properties with no reference to the algorithm, and RecordsConfig.cc:863 defaults ram_cache.algorithm to 1, so the default policy was the one ignoring the flag.

I checked a couple of other cases:

Tested on a 5000 byte object to exercise the size rounding in iobuffer_size_to_index(). The tests still pass. Worth using a non-power-of-two payload anyway, so the tests do not depend on 8192 landing exactly on a size index.

Checked the ram_cache_bytes gauge across the resident refresh: N objects put with copy=false, re-put with copy=true, then the cache overflowed to force eviction. The gauge rises on the puts and does not go negative through eviction, for all three policies. That path is the only place the fix does arithmetic rather than just swapping the buffer, so it is worth a permanent test.

I also recommend an S3-FIFO ghost admit test. The copy handling is inherited -- the ghost is removed and the fresh entry goes through the same insert site -- but the readmit lands in SEG_MAIN and accounts against _m_bytes, and with copy set the charged size is len rather than data->block_size(). That combination is not covered.

Happy to approve once it comes out of draft.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants