phongn commented on PR #13257:
URL: https://github.com/apache/trafficserver/pull/13257#issuecomment-5686130529
Thanks — good catch on the range check, that one would have shipped a
feature nobody could turn on. All six points are addressed in 421b518aac.
Details, including two places where I went past what you asked and one where I
deliberately didn't.
## Blocking: records range check — fixed
`[0-3]` → `[0-5]`. Confirmed the mechanism end to end before changing it:
`RECC_INT` goes to `recordRangeCheck()`, which parses the pattern as a numeric
min-max (not a character class — `[131072-104857600]` elsewhere in the file
makes that unambiguous), and `RecYAMLDecoder.cc:167` on failure takes the
`ERRATA_WARN` path and `return`s without ever calling `RecSetRecord`, so the
record keeps its `"0"` default. Exactly as you described.
For the drift guard I used a records unit test rather than an autest:
```cpp
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 `CACHE_COMPRESSION_*`
values, so adding a codec without widening the pattern fails the build's own
test rather than depending on someone remembering to extend a config; and it
follows the existing `search_default_domains accepts documented values` test
immediately above it, which does the same `GetRecordElementByName` +
`record->check`/`record->regex` dance. Cost is one new include —
`iocore/cache/Cache.h` into a records unit test. Those are header-only macros
so it links clean in every config I tried, but if you consider that layering
objectionable I'll hardcode `0..5` with a comment instead; it just loses the
automatic-failure property.
This one I did verify red-without-fix: reverted the pattern to `[0-3]`,
rebuilt, and the test fails pointing at value 4.
## Sticky-null zstd context — fixed, and the fix is bigger than the one you
suggested
`goto Lcontinue` was right about intent, but taking it literally at the
per-entry level would have traded a silent degradation for a worse operational
problem, so I want to flag the reasoning rather than bury it.
`init()` schedules the compressor with `eventProcessor.schedule_every(...,
HRTIME_SECOND, ET_TASK)`. Periodic events are re-enqueued on the same `EThread`
(`UnixEThread.cc`, the `if (e->period)` branch), so a given cache's compressor
runs on exactly one ET_TASK thread for the process lifetime. The context
failure is `thread_local` and sticky. So for a cache whose compressor landed on
the unlucky thread, "leave the entry eligible" means: every second, walk up to
`compress_percent` of the entries, and for each one drop the stripe lock,
`ats_malloc(ZSTD_compressBound(e->len))`, fail, free, retake the lock. Forever.
That is a lock-thrashing and allocator-churn regression against the current
behaviour, which at least gives up quietly after one pass.
So `compress_entries()` now checks once at the top and skips the whole pass:
```cpp
#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;
}
#endif
```
Entries are untouched, so they stay eligible exactly as you wanted; one
counter increment per skipped pass keeps it visible without flooding. The
per-entry `no_context` handling is still there underneath as defence in depth,
with a comment saying the pass-level check makes it unreachable for zstd today.
`ram_cache.compress.failure` added, global and per-volume, documented on
both stats pages. It counts codec errors and context-allocation failures; it
deliberately does **not** count objects that only failed `required_shrink`,
since that is the normal outcome for already-compressed content and would drown
the signal.
## Decode failures — codec error now reported
All seven `goto Lfailed` sites in `get()` set a detail string first. zstd
uses `ZSTD_getErrorName()`, libz uses `zError()`, and the rest format their
numeric return plus what was expected, e.g. `LZ4_decompress_safe returned %d,
expected %d` — which separates a malformed frame (negative) from `e->len`
disagreeing with the frame (smaller non-negative), which was your actual
question.
One wording change while I was in there: the `default:` arm said
"compression type not supported by this build", but
`CacheProcessor::cacheInitialized()` already `Fatal`s on a compiled-out
backend, so reaching that arm means a corrupt `flag_bits.compressed`, not a
build-option problem. It now says "no decoder for this compression type".
## Tests — strengthened, with one honest limitation
Incompressible case asserts `size_after == size_before`; single-byte case is
now parametrized over `compression_cases()` like the others, so lz4 and zstd on
1-byte input are covered (they all converge on `RAM_HIT_COMPRESS_NONE`, whether
via fastlz's `len < 16` guard, the `incompressible` marking, or the
verbatim-store path).
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 `size()`. With a
padded payload the raw re-store would legitimately *shrink* the entry, so
there's no assertion that covers both. The comment in the test now states this
rather than claiming it catches the raw-copy case.
You're also right that nothing discriminates the liblzma change, and for a
stronger reason than the test being weak: `lzma_easy_buffer_encode()` is handed
the output size and returns `LZMA_BUF_ERROR` rather than overrunning, so the
old code was never unsafe, and old-fails-encode and new-stores-raw are
indistinguishable at the `get()` boundary. I've rewritten that bullet in the
description to call it a robustness fix and to spell out that it is neither a
memory-safety nor a memory-saving change.
## static_asserts — fixed, with one deviation
Bitfield assert moved to `RamCacheCLFUS.h` directly under the field,
unconditional. Verified it is now actually evaluated without zstd: in a
`-DCMAKE_DISABLE_FIND_PACKAGE_ZSTD=ON` build, temporarily falsifying it does
fail the compile, which it could not do from inside `#ifdef HAVE_ZSTD_H`.
Added your count assert. I did **not** remove the six pairwise ones, though:
they catch a reordering of either sequence (reorder the `RAM_HIT_COMPRESS_*`
enumerators and the count assert still passes while the pairwise ones fire),
whereas yours catches a `CACHE_COMPRESSION_*` added without its enumerator.
Different failures, both cheap, so I kept both and wrote a comment saying which
does what. Happy to drop the pairwise block if you'd still rather have just the
one.
## Destructor and the scheduled compressor — took the comment option
I 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. `RamCacheCLFUSCompressor` is constructed with no mutex, so
`Event::cancel()` from the destructor's thread races a `compress_entries()`
already executing on the ET_TASK thread — you'd close the "fires again later"
window and leave the "currently running against a half-destroyed object" one
open. Making it genuinely safe means giving the compressor `stripe->mutex` and
requiring the destructor to hold it, which changes production scheduling
behaviour (the compressor would then contend and reschedule on lock miss) for a
path production never takes — these live as long as their `StripeSM`.
`~RamCacheCLFUS()` now carries that reasoning, and the stale comment in
`test_RamCacheCompressEntries.cc` is corrected. Also wired both failure
counters in that test, which had been left with null metric pointers.
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.
## Verification
Three configurations, all green: default, `ENABLE_ASAN` (clean — the June
leak and use-after-free stay fixed), and
`-DCMAKE_DISABLE_FIND_PACKAGE_{LZ4,ZSTD,LibLZMA}=ON` to exercise the
compiled-out paths. Full-tree build is warning-clean.
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 `WARN`ed about it).
apache/trafficserver-ci#441 is still the thing that closes that gap in CI.
## Deliberately not in this PR
The libz and liblzma decode paths check only the library return code, never
that the output length equals `e->len` — fastlz, lz4 and zstd all do.
Pre-existing and outside this diff, and the right place to fix it once for
every codec is the shared `decompress()` helper in the follow-up refactor, so
I've queued it there rather than adding a sixth thing here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
