Conversation
bryancall
left a comment
There was a problem hiding this comment.
Took a pass over this — really nice change. The liblzma lzma_stream_buffer_bound() fix is a real correctness improvement, the per-thread zstd context design is clean, and adding compression-integrity tests where there were none is great. A handful of improvement suggestions inline; none are blockers.
The one I'd most encourage looking at is the sticky-null behavior of the thread-local zstd context: a one-time allocation failure silently degrades that thread to uncompressed forever (and evicts valid entries on read) with no log or metric. Details inline.
|
Since I know you probably cant get to the CI output @phongn , here's the failure. It's in the clfus catch test: |
|
Related PR to add zstd and lz4 to our CI hosts: apache/trafficserver-ci#441 |
5d3ea03 to
aa2a6a8
Compare
cmcfarlen
left a comment
There was a problem hiding this comment.
I think this looks good. I have a couple of concerns.
- Will this build if LZ4 or ZSTD are not found?
- I don't think CLFUS is used due to other concerns with the technique. Does this PR resolve those?
requesting changes just to get answers to the questions.
|
There was a problem hiding this comment.
The threads from my June review are all resolved and CI is green, so the only thing blocking this now is the merge conflict.
Please rebase onto master and I will do the full review. With 22 files touched, including CMakeLists.txt, both Dockerfiles and the new FindLZ4.cmake / FindZSTD.cmake modules, I would rather review the rebased tree than a version that cannot land.
Marking this as request changes until it is mergeable again. That is not a comment on the code itself, which looked good in June.
RamCacheCLFUS allocated its hash table, seen filter, and entries but had no destructor, so destroying an instance leaked them. LeakSanitizer flagged this once the new unit test started creating and destroying instances. Add a destructor that releases each entry's data, returns the entries to the allocator, and frees the table and seen filter. The synchronous RAM cache test never calls TEST_DONE(), so the event threads kept running as the process tore down static state at exit and an ET_NET thread incremented the freed Metrics singleton (heap-use-after-free). Shut the event system down from the shared cache test harness at end of run so the threads stop first. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
bryancall
left a comment
There was a problem hiding this comment.
❌ Request changes
Thanks for the rebase. The June threads are all addressed and the rebased tree looks good, but one thing outside the diff blocks the feature from working as shipped.
Blocking: the records range check still rejects 4 and 5
src/records/RecordsConfig.cc line 889 defines proxy.config.cache.ram_cache.compress with the validity pattern [0-3]. RecYAMLDecoder runs that check on load, logs a validity warning on failure, and uses the default of 0. So setting 4 or 5 in records.yaml, as the docs now tell people to, silently leaves compression off. The unit test does not catch it because it writes cache_config_ram_cache_compress directly. Changing the pattern to [0-5] is the fix. It would be worth a one-line autest or gold-test config that sets the record to 5 and checks traffic_layout or a startup log line, so the range check cannot drift again.
Fixed: sticky-null zstd context
The one-time warning in zstd_cctx() / zstd_dctx() covers what I asked for. One non-blocking refinement: when zstd_cctx() returns null in compress_entries(), failed = true lands on Lfailed, which sets incompressible on the entry. That records a thread-level allocation condition as a permanent property of the data, which is the opposite of the (correct) choice you made in get(). goto Lcontinue there would leave the entry eligible for a later pass. A ram_cache.compress.failure counter next to the new decompress one would also make this diagnosable without log access.
Fixed: decode failures are now visible
The throttled warning plus the global and per-volume ram_cache.decompress.failure counters, with the null-context case treated as a miss, is exactly right. Non-blocking: the codec's own error is dropped on the floor in every branch. Capturing ZSTD_getErrorName() or the negative lz4 return into a local and printing it in the Lfailed warning would tell an operator whether a nonzero counter means a corrupted frame or a bookkeeping bug in len.
Fixed: test proves compression happened
256 KB payload and size_after < size_before for every non-NONE backend. Two non-blocking gaps: the incompressible test does not assert size_after == size_before, so a regression that stored a raw copy instead of marking the entry would still pass, and the single-byte case runs only under CACHE_COMPRESSION_NONE, so lz4 and zstd on tiny input (both emit frames larger than 1 byte) are untested. Parametrizing it over compression_cases() like the others would close that. Also, nothing discriminates the liblzma lzma_stream_buffer_bound change; the old code failed the encode and the new code stores raw, and both read back as NONE. Fine to leave, but the description should call it a robustness fix rather than a memory fix.
Fixed: static_asserts
Two non-blocking notes. static_assert(CACHE_COMPRESSION_ZSTD < (1 << 3)) sits inside #ifdef HAVE_ZSTD_H in the .cc, so a build without zstd never evaluates it; it belongs in RamCacheCLFUS.h directly under the bitfield, unconditional. The six pairwise asserts in Cache.h are each true by construction and would not fire if someone added CACHE_COMPRESSION_FOO 6 without the matching enumerator; one static_assert(RAM_HIT_LAST_ENTRY == CACHE_COMPRESSION_ZSTD + 2) would.
Non-blocking: the new destructor and the scheduled compressor
~RamCacheCLFUS() frees the entries, buckets and seen filter, but init() schedules a RamCacheCLFUSCompressor holding a raw back-pointer that nothing cancels. Production never destroys these objects, and the test sidesteps it by initializing with compression off, so this is latent. Either keep the Event * and cancel it in the destructor, or say in a comment that the destructor is only safe when compression was never scheduled. The comment in test_RamCacheCompressEntries.cc saying the policy has no destructor is now stale.
Not verified by running it
The new test cannot be run at the base commit since it depends on symbols this patch adds, so there is no red-without-fix result to report. CI is green on the rebased head.
Allow the new codecs to actually be configured. The validity pattern for proxy.config.cache.ram_cache.compress was still [0-3], so RecYAMLDecoder rejected 4 and 5 at load time, logged a validity warning and fell back to the default of 0 -- leaving compression off for exactly the two backends the docs now recommend. Widen it to [0-5] and add a records unit test that walks every CACHE_COMPRESSION_* value against the record's own check and pattern, so the range cannot drift behind the enum again. Distinguish a missing zstd context from incompressible data. A null ZSTD_CCtx is a thread/allocator condition, so recording it as the entry's permanent incompressible flag was the opposite of the choice already made in get(). The entry is now left eligible for a later pass. Because that failure is sticky and the compressor event is pinned to one ET_TASK thread, leaving entries eligible alone would turn a one-time allocation failure into a walk over most of the RAM cache every second -- dropping and retaking the stripe lock and allocating a compressBound()-sized buffer per entry, only to fail each time -- so compress_entries() now skips the whole pass on a thread with no context. Real compression failures and skipped passes increment a new ram_cache.compress.failure counter (global and per-volume) so they are diagnosable without log access; objects that merely did not shrink enough are deliberately not counted, since that is the ordinary outcome for already-compressed content. Report the codec's own error on a decompression failure. Every branch dropped it, so a nonzero decompress.failure counter could not tell a corrupt frame from a bookkeeping error in e->len. The throttled warning now carries ZSTD_getErrorName(), zError(), or the codec's numeric return. Move the bitfield static_assert next to the field it guards, in RamCacheCLFUS.h and unconditional; inside #ifdef HAVE_ZSTD_H it was never evaluated by a build without zstd. Add one count assert in Cache.h that fires if a CACHE_COMPRESSION_* is added without its RAM_HIT_COMPRESS_* enumerator; the pairwise asserts are kept because they catch a reordering of either sequence, which the count assert does not. Strengthen the compression tests: the incompressible case now asserts the entry's footprint is unchanged (the 256 KB payload is a power of two, so there is no padding for the pass to legitimately reclaim), and the single-byte case is parametrized over every backend instead of running only under CACHE_COMPRESSION_NONE. Document why ~RamCacheCLFUS() is only safe for a cache that never scheduled the background compressor, and correct the now-stale comment in test_RamCacheCompressEntries.cc. Cancelling the event is not enough to fix this: the continuation carries no mutex, so it can run compress_entries() concurrently with the destructor. Making that safe means giving the compressor the stripe mutex and requiring the destructor to hold it, which is not worth it while production never destroys a RamCacheCLFUS. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Thanks — good catch on the range check, that one would have shipped a feature nobody could turn on. All six points are addressed in 421b518. Details, including two places where I went past what you asked and one where I deliberately didn't. Blocking: records range check — fixed
For the drift guard I used a records unit test rather than an autest: TEST_CASE("ram_cache.compress accepts every compression backend", "[librecords][RecUtils]")
{
const auto *record = GetRecordElementByName("proxy.config.cache.ram_cache.compress");
...
for (int i = CACHE_COMPRESSION_NONE; i <= CACHE_COMPRESSION_ZSTD; i++) {
INFO("CACHE_COMPRESSION_* value: " << i);
REQUIRE(RecordValidityCheck(std::to_string(i).c_str(), record->check, record->regex));
}
REQUIRE_FALSE(RecordValidityCheck(std::to_string(CACHE_COMPRESSION_ZSTD + 1).c_str(), record->check, record->regex));
}Two reasons over a gold test: it iterates the real This one I did verify red-without-fix: reverted the pattern to Sticky-null zstd context — fixed, and the fix is bigger than the one you suggested
So #ifdef HAVE_ZSTD_H
if (cache_config_ram_cache_compress == CACHE_COMPRESSION_ZSTD && zstd_cctx() == nullptr) {
ts::Metrics::Counter::increment(cache_rsb.ram_cache_compress_failures);
ts::Metrics::Counter::increment(stripe->cache_vol->vol_rsb.ram_cache_compress_failures);
return;
}
#endifEntries are untouched, so they stay eligible exactly as you wanted; one counter increment per skipped pass keeps it visible without flooding. The per-entry
Decode failures — codec error now reportedAll seven One wording change while I was in there: the Tests — strengthened, with one honest limitationIncompressible case asserts On the footprint assertion — it is weaker than it looks and I'd rather say so than let it read as more than it is. It catches "stored the expanded compressed blob", but it cannot distinguish "marked incompressible and left alone" from "re-stored a raw copy", because the 256 KB payload is a power of two and carries no buffer padding, so both land on the same You're also right that nothing discriminates the liblzma change, and for a stronger reason than the test being weak: static_asserts — fixed, with one deviationBitfield assert moved to Added your count assert. I did not remove the six pairwise ones, though: they catch a reordering of either sequence (reorder the Destructor and the scheduled compressor — took the comment optionI went with documenting the constraint rather than cancelling, because cancelling isn't sufficient and I didn't want to ship something that looks safe and isn't. If you'd rather have the mutex change, I'd prefer it as its own PR against the shared-compression refactor, where the compressor continuation moves anyway. VerificationThree configurations, all green: default, Worth noting for the record: this host has lz4 1.9.3 and zstd 1.5.5, so all six cases actually ran (79 assertions, versus the 45 you'd have seen in the Rocky CI log where lz4 was missing and the test Deliberately not in this PRThe libz and liblzma decode paths check only the library return code, never that the output length equals |
clang-analyzer flagged the "no detail" initializer for codec_error as a dead store, correctly: every path to Lfailed assigned a real detail string first, so the sentinel was never read. Dropping just the initializer would have left a genuine uninitialized read the first time someone added a goto Lfailed without setting it. The sentinel and the function-scope char buffer only existed because the detail string had to survive a goto, so report the failure at each site instead. note_decompress_failure() now carries the throttled warning and both counters, each branch formats its detail in its own scope, and Lfailed is back to freeing the buffer and destroying the entry. Behavior is unchanged: one process-wide throttler, and the entry is still intact when the warning reads its fields. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Follow-up in aaf29f1 for the two CI failures on 421b518. Clang-Analyzer — fixed
Correct, and the sentinel was load-bearing for nothing: all seven paths to Instead I removed the reason the sentinel existed. The if (l != rc) {
char detail[128];
snprintf(detail, sizeof(detail), "LZ4_decompress_safe returned %d, expected %d", rc, l);
note_decompress_failure(stripe, key, e, detail);
goto Lfailed;
}A new file-local This supersedes the Verified locally with AuTest 2of4 — not this PRThe only failing test in that shard is That is #13679, opened today: the queue precondition races a TLS handshake on a 0.3s timer. The shard was also heavily loaded on this run (worker 1 took ~1456s), which is the condition that issue describes. Nothing in this PR reaches the SNI rate limiter — the diff is RAM cache compression, a records validity pattern, and two counters — and every other shard passed, as did all nine platform builds. I'd rather not rebase just to reroll the dice on a known flake, so unless you want it rerun for a clean board, the new push should sort it on its own. |
None of these change RAM cache behavior. The zstd config-mode alias block could never fire, the decompression warning reimplemented the site-wide log throttling it should have called, and the compress.failure doc claimed a per-entry count that the pass-level skip does not produce. The rpm spec and the contrib images also lacked lz4 and zstd, so a documented compress value would be Fatal at startup on those builds. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Several were mine and hid real behavior: compress.failure counted skipped passes as entry failures, a transient codec allocation error marked an entry permanently incompressible, the destructor skipped the byte accounting _destroy() does, and the new destructor left the copy operations implicit. Codec validation now happens once where the value is read instead of twice after the compressor is already scheduled, and the Find modules are the tree's own rather than a BSD import. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Correcting something I told you in my earlier reply, since the claim is wrong and it is load-bearing for a decision you may make later. On the destructor thread, I wrote:
The stripe mutex is exactly the wrong mutex for this, and not for the reason I gave. The right shape is the one used elsewhere in the tree: give Worth being explicit about why the cancel is needed at all, which I was vague about: I have not made that change here — it is a behavioural change to production scheduling and belongs with the shared-compression refactor, where the compressor continuation moves anyway. I have queued it there, and corrected the comment in |
bryancall
left a comment
There was a problem hiding this comment.
✅ Approving. The blocker from my last review is fixed and it now has a test that catches it.
I rebuilt and ran this rather than reading it: configured 132ddc21 on macOS/clang with lz4 1.10.0 and zstd 1.5.7, ran RamCacheCLFUS (79 assertions, all six backends) and test_records "[RecUtils]", then reconfigured with both CMAKE_DISABLE_FIND_PACKAGE_* set and reran. Everything I call verified below was run.
I reverted src/records/RecordsConfig.cc:889 to [0-3], rebuilt, and got test_RecUtils.cc:235: FAILED ... CACHE_COMPRESSION_* value: 4. Restored: 89 assertions green. The upper bound is 5 and CACHE_COMPRESSION_ZSTD is 5, so there is no off-by-one hiding zstd.
All of my earlier asks have landed. Two worth calling out as better than what I asked for: the null-cctx handling became a pass-level skip at RamCacheCLFUS.cc:549 plus a transient flag routing allocation errors to Lcontinue, and the error-text capture covers all five codecs rather than the new two. On the incompressible case you used size_after <= size_before instead of == with a comment about padding: I ran it and it is exactly equal at 262280 both sides, your reasoning is right, leave it.
Also verified, since these are the things most likely to be wrong in a change like this: the zstd contexts are thread_local std::unique_ptr with ZSTD_freeCCtx/ZSTD_freeDCtx deleters so they release at thread exit rather than leaking, a per-call ZSTD_CCtx_reset() is genuinely unnecessary because ZSTD_compress2() starts a fresh frame and the level is sticky, the read path treats every codec failure as a miss with the entry intact and no double free, and buffer sizing uses LZ4_compressBound() / ZSTD_compressBound() with the codec's own return stored rather than the bound.
Non-blocking
1. Deleting the zstd config-mode shim is a regression against master
a8a1c45a removed the alias block because it "could never fire". It does fire.
With CMAKE_FIND_PACKAGE_PREFER_CONFIG=ON, find_package(ZSTD 1.4.0) at CMakeLists.txt:496 resolves through zstd's own config package instead of cmake/FindZSTD.cmake. ZSTD_FOUND is set so HAVE_ZSTD_H is true, but the exported target is zstd::libzstd_shared, not zstd::zstd. cmake/FindZSTD.cmake:70 never runs, nothing creates the alias, and the four places linking zstd::zstd fail at generate time (src/iocore/cache/CMakeLists.txt:66, src/iocore/net/CMakeLists.txt:137, src/traffic_layout/CMakeLists.txt:35, plugins/compress/CMakeLists.txt:29).
Reproduced standalone against this tree's cmake/ directory:
-- PREFER_CONFIG ZSTD_FOUND=1 ZSTD_CONFIG=/opt/homebrew/lib/cmake/zstd/zstdConfig.cmake
-- TARGET zstd::libzstd_shared EXISTS
-- *** zstd::zstd MISSING
CMake Error at CMakeLists.txt:10 (target_link_libraries):
zstd::zstd
It is a non-default setting and no ATS CI job sets it, which is why CI is green, so I am not blocking. I would still fix it before merge: it is four lines after find_package(ZSTD 1.4.0), aliasing whichever of zstd::libzstd_shared / zstd::libzstd_static / zstd::libzstd exists.
The description still says the alias shim is kept for CMAKE_FIND_PACKAGE_PREFER_CONFIG builds. Please either restore it or drop that sentence, and fix the description before re-requesting review, since the next reviewer reads it alongside the diff.
2. Nothing tests the decompress failure paths
note_decompress_failure() at RamCacheCLFUS.cc:295 and its five call sites are the visible part of this change on the read path, and no test reaches any of them. I proved it: replacing the ZSTD_isError(ll) check at line 398 and the l != rc check at line 377 with if (false) still gives "All tests passed (79 assertions in 4 test cases)". Deleting both codecs' error detection is invisible to the suite.
A corrupted-frame case closes it: store, run compress_entries(), flip a byte in the stored blob, get(), then assert a miss, an intact entry, and ram_cache_decompress_failures == 1. The entries are private so it needs a small test hook or a friend declaration. Worth it, because the counter and the warning are the operator-facing half of this feature and right now they are only proven to compile.
3. A real round-trip failure will report as a stringification crash
CHECK(r.out == payload) at test_RamCacheCLFUS.cc:206, 238 and 265 compares two 256 KB std::vector<char>. Catch2 stringifies both operands to report an assertion and on this payload that throws. ./RamCacheCLFUS -s gives due to unexpected exception with messages: compression backend: zstd / basic_string.
Green without -s, so not a failing test today. The problem is the day a codec genuinely breaks the round trip and CI shows unexpected exception: basic_string instead of which backend and which byte. Compare into a bool first, or assert on sizes plus memcmp.
4. The lz4 version floor is below the API the tree uses
CMakeLists.txt:501 sets 1.7.0, but src/traffic_layout/info.cc:273 calls LZ4_versionString(), which lz4.h documents as requiring v1.7.5+. Against 1.7.0 through 1.7.4 the find module reports success and traffic_layout then fails to link. Raise the floor to 1.7.5, or print only LZ4_VERSION_STRING. The zstd 1.4.0 floor is correctly enforced: find_package(ZSTD 99.0.0) gives Could NOT find ZSTD: Found unsuitable version "1.5.7".
5. The Fatal message does not say what to change
Cache.cc:884 and 889 say "lz4 not available for RAM cache compression". The operator hitting this just set a number in records.yaml and now has a process that will not start. Name the record and the value the way the default: case at line 893 does.
6. Smaller things
NOTICEgains one trailing blank line and no content. Drop it from the diff.ci/docker/yum/Dockerfilerenameszstd-develtolibzstd-develon a line unrelated to lz4. Probably the right Fedora name, but it is an unrelated change riding along.doc/developer-guide/cache-architecture/ram-cache.en.rstfixesuncompressibletoincompressibleand leaves "it mat be compressed" on the next line.traffic_layout info --versionsprints zstd as the runtime version and lz4 as the compile-time one with a separatelz4.run. Harmless, but similar names now mean different things.
One thing to answer before this merges
@cmcfarlen's dismissed review asked two questions. The first, whether this builds without lz4 or zstd, I have verified myself: it does, cleanly, and the test skips with a WARN rather than failing. The second, whether this addresses the other concerns with the CLFUS technique, has no answer anywhere in the thread or the description. He is a requested reviewer again, so please answer it in the PR rather than leaving my approval to carry the change past it.
On CI
All 14 checks pass on 132ddc21. One caveat worth stating: the unit test WARNs rather than failing when a backend is absent, so a green run does not by itself prove the new code was exercised anywhere in CI, and your own description notes the Fedora image still needs lz4-devel. That is why I built and ran it locally.
Restoring the config-mode alias is a fix for my own regression: with CMAKE_FIND_PACKAGE_PREFER_CONFIG the lookup resolves through zstd's config package on a case-insensitive filesystem, which exports zstd::libzstd_shared rather than zstd::zstd, so deleting the alias broke macOS builds at generate time. The new corrupted-frame test reaches the decompression failure paths, which nothing exercised before, and the round-trip checks no longer hand Catch2 two 256 KB operands to stringify, which threw instead of reporting the mismatch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Thanks — and thank you for rebuilding it rather than reading it. All six are addressed in 1. The zstd shim — you are right, and it was my regressionI deleted it on a Linux-only check and generalised. CMake's config-mode search looks for I reproduced it here rather than taking it on faith, by simulating the config package on Linux and toggling only the filename:
So Restored as a loop over Description fixed too, and expanded — the sentence you flagged was true again once the shim came back, but it did not say why the shim exists, which is how it got deleted. It now spells out the case-insensitive filename match. 2. Decompress failure paths — test added, and it earned its placeYou were right that nothing reached them. It found more than coverage: it exercises the per-codec detail text for real, and all five produce a distinct, useful message. I re-ran your mutation. 3. The 256 KB compare — worse than green-without-
|
Add lz4 and zstd compression support to the CLFUS RAM cache
Summary
This adds two modern compression backends to the CLFUS RAM cache, as optional build-time dependencies:
proxy.config.cache.ram_cache.compress: 4) — a replacement for fastlz: strictly better compression ratio at substantially higher throughput.proxy.config.cache.ram_cache.compress: 5, level 3) — a replacement for libz/deflate-6: comparable ratio at roughly 10× the compression speed and 3× the decompression speed.The existing fastlz/libz/liblzma backends are unchanged and remain valid for their config values; the docs now recommend lz4 over fastlz and zstd over libz.
Benchmarks (lzbench, silesia XML corpus, Xeon Gold 6338, one thread):
Implementation notes
LZ4_compress_default/LZ4_decompress_safeAPI.thread_localZSTD_CCtx/ZSTD_DCtx) withZSTD_compress2and a sticky compression level, avoiding a context allocation per call — relevant since decompression sits on the cache-hit path. This requires zstd ≥ 1.4.0 (the first release with the advanced one-shot API stable); the version floor is enforced infind_package.Fatal, matching the existing liblzma behavior.cmake/FindLZ4.cmakeandcmake/FindZSTD.cmakemodules, written in the same shape as the tree's otherFind*.cmake(Apache header,find_library/find_path,INTERFACE IMPORTEDtarget) plus the header-version parse needed to enforce a floor. zstd detection previously usedfind_package(zstd CONFIG)only, which fails on distributions that don't ship a cmake config package; it now resolves via the module.zstd::zstdalias shim is kept, and it is not dead code. WithCMAKE_FIND_PACKAGE_PREFER_CONFIGthe lookup can resolve through zstd's own config package rather than the module: CMake searches forZSTDConfig.cmake, and on a case-insensitive filesystem that matches thezstdConfig.cmakezstd installs. That package exportszstd::libzstd_shared/_static, so without the alias the four targets linkingzstd::zstdfail at generate time on macOS. The shim now also disables zstd with a warning if the config package exports none of the names it knows, rather than failing the generate.LZ4_compress_default(), andLZ4_versionString(), whichtraffic_layoutreports, needs 1.7.5.traffic_layout infonow reportsTS_HAS_LZ4, and--versionsreports the lz4 runtime version underlz4, matching what the existingzstdkey means.Testing
The
RamCacheCLFUSclass definition moved from the.ccinto a new private header so unit tests can drivecompress_entries()synchronously. A new Catch2 test (test_RamCacheCLFUS) does store → compress → read-back roundtrips across all five codecs plus the uncompressed case, asserting byte-for-byte equality and the expectedRAM_HIT_COMPRESS_*state, plus incompressible-fallback and small-payload cases. No prior test verified compression data integrity for any backend.Drive-by fixes
e->leninstead oflzma_stream_buffer_bound(e->len); it now matches the other backends, with the existingREQUIRED_COMPRESSION/REQUIRED_SHRINKthresholds deciding what to keep. To be precise about what this fixes: it is a robustness fix, not a memory-safety or memory-saving one.lzma_easy_buffer_encode()is told the output size and returnsLZMA_BUF_ERRORrather than overrunning, so the old code was never unsafe — it just failed the encode for any object that didn't shrink, and that failure marked the entryincompressible. Old code and new code both end up reading back asRAM_HIT_COMPRESS_NONEfor such an object (the old one by failing, the new one by storing it raw), so no test can discriminate the two; what changes is that objects liblzma can compress but whose bound exceedse->lennow get compressed instead of being written off.proxy.config.cache.ram_cache.compresshad validity pattern[0-3]inRecordsConfig.cc, soRecYAMLDecoderrejected the new values 4 and 5 at load time, logged a validity warning and fell back to the default of0. Without this, setting4or5inrecords.yamlas the docs describe would silently leave compression off. Widened to[0-5], with a unit test that walks everyCACHE_COMPRESSION_*value against the record's own check and pattern so the range cannot drift behind the enum again.compress: 2was configured — zlib is a required dependency and always available.Observability
Two new counters, global and per-volume:
ram_cache.decompress.failure— an entry failed to decompress on read. The entry is dropped and the read becomes a miss, and a throttledWarningcarries the codec's own diagnosis (ZSTD_getErrorName(),zError(), or the codec's numeric return) so a nonzero counter can be told apart from a corrupt frame versus a bookkeeping error ine->len. Previously any decode failure was indistinguishable from an ordinary miss outside a debug build.ram_cache.compress.failure— an entry the compression library could not compress. Objects that merely did not shrink enough are deliberately not counted; that is the ordinary outcome for already-compressed content and would swamp the signal. A thread whose zstd context cannot be allocated skips its compression pass entirely and is not counted here either, since that would make the counter climb once a second per stripe for the life of the process; the one-timeWarningreports that condition.CI / packaging
liblz4-dev/lz4-develadded to the deb and yum CI images. The Fedora CI image will needlz4-develadded for build coverage of the new backend (zstd-devel is already present).Future work
Compression is currently implemented inside CLFUS only. A follow-up refactor will extract it into a shared layer so all RAM cache algorithms can use it; the new unit test is structured to migrate there.