Skip to content

Add regression test for string_fastpath/compression flag collision#1098

Closed
mhenrixon wants to merge 1 commit into
petergoldstein:mainfrom
mhenrixon:add-fastpath-compression-regression-test
Closed

Add regression test for string_fastpath/compression flag collision#1098
mhenrixon wants to merge 1 commit into
petergoldstein:mainfrom
mhenrixon:add-fastpath-compression-regression-test

Conversation

@mhenrixon

Copy link
Copy Markdown
Contributor

Adds an integration test covering the collision between ValueSerializer::FLAG_UTF8 and ValueCompressor::FLAG_COMPRESSED. Both constants are 0x2, so any value written with string_fastpath: true as a per-request option produces ambiguous flags on read.

The bug is fixed by #1086, but no test was added there that fails on main and passes on the fix — so the regression could re-emerge if the flag handling is refactored again. This PR fills that gap.

What the test covers

  • Short UTF-8 string with string_fastpath: true per-request + compress: true client-level. Below the compression threshold, but the fastpath still sets bit 0x2, which the reader's compressor interprets as FLAG_COMPRESSED and tries to Zlib.inflate on raw UTF-8 bytes.
  • Long UTF-8 string (over compression_min_size).
  • Long BINARY string — fastpath leaves bit 0x2 clear, compressor sets it; without the fix the reader returns the decompressed bytes with the wrong encoding.
  • A second client that never opts into string_fastpath reading every key — covers a rolling-deploy or worker-pool mismatch where only some processes use the per-request option (mirrors how ActiveSupport::Cache::MemCacheStore#write_serialized_entry forwards per-call options to Dalli).

Why the existing fastpath test didn't catch it

test/integration/test_serializer.rb#L17 covers string_fastpath as a client-level option without compression. The new test covers the production-relevant path: string_fastpath: true as a per-request option on #set, with compress: true on the client. That's the combination Rails callers naturally produce — Rails strips :compress from per-call options (see active_support/cache/mem_cache_store.rb#L215) but forwards everything else.

Verification

On main (1bc7403, "Prepare 5.0.3 release"):

Serializer configuration::using the meta protocol#test_0003_round-trips fastpath strings when compression is enabled (per-request option) = 0.11 s = E

  1) Error:
Serializer configuration::using the meta protocol#test_0003_round-trips fastpath strings when compression is enabled (per-request option):
Dalli::UnmarshalError: Unable to uncompress value: incorrect header check
    lib/dalli/protocol/value_compressor.rb:48:in 'Dalli::Protocol::ValueCompressor#retrieve'
    ...

5 runs, 22 assertions, 0 failures, 1 errors, 0 skips

With #1086 applied (cherry-picked locally):

5 runs, 34 assertions, 0 failures, 0 errors, 0 skips

Production symptom this corresponds to

When the UTF-8 branch fires inside Rails, Dalli::UnmarshalError is caught by MemCacheStore#rescue_error_with and returned as a silent cache miss. The application re-runs whatever produced the value and re-writes it; the next read trips the same error. The Memcachier dashboard for an app that hit this shows roughly equal gets, misses, and sets at very high volume with hit rate hovering near 80% — looks healthy at a glance, but every cacheable computation is running twice or more. See #1086 (comment) for the full standalone repro and version matrix.

Notes

@petergoldstein

Copy link
Copy Markdown
Owner

Thanks for clarifying the issue and providing a test. Test included in #1099.

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.

2 participants