Potential breaking change: digest memcached cache keys - #99
Conversation
The cache key was the JSON-serialized request descriptor, which carries the url and query parameters verbatim. Memcached keys must be ASCII, free of whitespace and at most 250 bytes long. Dalli's meta protocol, which replaced the binary protocol in Dalli 5, works around the first two constraints by base64-encoding keys that violate them. That inflates the key by a third, and since Dalli checks the length before encoding, keys longer than 187 bytes end up over the 250 byte limit and memcached replies CLIENT_ERROR. Any request with whitespace or non-ASCII in its parameters was therefore at risk. The binary protocol sent keys as length-prefixed raw bytes, so whitespace and length were both fine and this only surfaced after upgrading Dalli. Digesting the descriptor keeps the key short and ASCII whatever the url and parameters contain. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
dcbc8dd to
e983f9e
Compare
|
@lleirborras checked in IATI staging and the fix worked! |
There was a problem hiding this comment.
Pull request overview
This PR hardens Hawk’s Memcached-backed HTTP caching against Dalli 5’s meta protocol key constraints by switching from raw JSON descriptor keys to a fixed-length digest, preventing whitespace/non-ASCII key encoding blowups that can exceed Memcached’s 250-byte limit.
Changes:
- Digest HTTP cache keys using
SHA256( MultiJson.dump(descriptor) )instead of using the serialized descriptor verbatim. - Add an RSpec matcher for “memcached-safe” keys and a new spec that verifies key safety and stability across requests (including non-ASCII params).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
lib/hawk/http/caching.rb |
Switches cache key generation from raw JSON to a SHA256 hex digest to guarantee a safe, bounded key. |
spec/spec_helper.rb |
Adds a custom matcher to assert cache keys are compatible with Memcached/Dalli meta protocol constraints. |
spec/hawk/http/caching_spec.rb |
Adds specs ensuring generated keys are safe (ASCII/whitespace-free/≤250 bytes), stable for identical requests, and distinct across different requests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <[email protected]>
tagliala
left a comment
There was a problem hiding this comment.
Reviewed with OpenAI Sol 5.6.
The production change correctly fixes the reported Dalli 5 cache-key failure, and the tests provide adequate coverage for the intended behavior.
The review identified two edge cases: stale entries can survive temporarily during a mixed-version rolling deployment because the key format changes, and an unusually long namespace containing whitespace or non-ASCII characters can still produce an oversized wire key after Dalli encoding. Neither is worth changing here: stale cache entries are acceptable for this deployment, and the namespace case is not representative of our configuration.
Approved.
The matcher is meant to model what memcached accepts verbatim, but it only rejected whitespace. Memcached also rejects keys containing ASCII control characters (0x00-0x1F and 0x7F), and Dalli's KeyRegularizer base64-encodes only non-ASCII or whitespace keys, so a control character is passed through as-is and the server replies CLIENT_ERROR. Widen the character class to /[[:cntrl:]\s]/ so the matcher describes memcached's constraints rather than Dalli's encoding trigger. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
tagliala
left a comment
There was a problem hiding this comment.
Re-reviewed the current head with OpenAI Sol 5.6. The follow-up correctly rejects control characters in the memcached-safe key matcher and keeps its failure message consistent. The production fix remains correct, and all current CI checks pass.
Approved.
Problem
With Dalli 5, a cached
GETwhose parameters contain whitespace or non-ASCII can fail:Root cause
The cache key was
MultiJson.dump(descriptor)— the url and query parameters verbatim.Memcached keys must be ASCII, whitespace-free and at most 250 bytes. Dalli 5 replaced
the binary protocol with the line-based meta protocol, which cannot carry whitespace, so
Dalli::Protocol::Meta::KeyRegularizerbase64-encodes such keys. That inflates them by athird — and
Dalli::KeyManager#validate_keychecks the 250-byte limit before encoding,so its md5-truncation fallback never kicks in. Any key over 187 bytes then goes on the
wire above 250 bytes and the server answers
CLIENT_ERROR.JSON serialization contributes no whitespace of its own, so this needs whitespace or
non-ASCII inside a parameter value — a person's name, a search string, an accented
place name. The binary protocol sent keys as length-prefixed raw bytes, so both
whitespace and length were harmless before the Dalli upgrade.
Fix
Digest the serialized descriptor. 64 hex chars, 69 bytes once the Dalli namespace is
prepended, never base64-encoded, whatever the url and parameters contain.
Verification
New
spec/hawk/http/caching_spec.rbasserts the key is ASCII, whitespace-free and≤250 bytes — including a non-ASCII parameter case — and that it is stable per request and
distinct across requests.
45 examples, 0 failures, RuboCop clean.End-to-end against dalli 5.0.5 + memcached 1.6.45, a raw 203-byte descriptor key raises
CLIENT_ERRORwhere its 64-byte digest round-trips.Note for consumers
The key format changes, so existing cache entries go cold on deploy. Default TTL is 60
seconds.
Companion fix in people-client, where this was actually erroring in production:
https://github.com/ifad/people-client/pull/138
🤖 Generated with Claude Code