Skip to content

Fix single-server set_multi key encoding - #91

Draft
mrattle wants to merge 1 commit into
Shopify:mainfrom
mrattle:mrattle/set-multi-key-handling
Draft

mrattle wants to merge 1 commit into
Shopify:mainfrom
mrattle:mrattle/set-multi-key-handling

Conversation

@mrattle

@mrattle mrattle commented Oct 23, 2026

Copy link
Copy Markdown

Summary

Single-server set_multi (the PipelinedSetter → write_multi_storage_req fast path) built keys differently from every other operation:

  • It dropped the base64 flag returned by KeyRegularizer.encode, so keys containing whitespace or non-ASCII characters were stored under their base64 text. get and get_multi never find them there.
  • It applied the namespace and truncation to the encoded key (Fix set multi namespace on single server #47) instead of the raw key.
  • It skipped to_s, so Symbol and Integer keys raised NoMethodError.

Multi-server rings were unaffected because they fall back to quiet { set }.

This moves key validation into PipelinedSetter, using the client's KeyManager in the same way as PipelinedGetter and PipelinedDeleter. write_multi_storage_req now only encodes the key and sends b. The per-server KeyManager added to Protocol::Base for this path had no other users and is removed.

Intended behaviour:

  • set_multi writes to the same keys that set, get, and get_multi use, for any ring size, with or without a namespace. Items already written under base64 text are never read and expire normally.
  • set_multi accepts the same non-String keys as set.
  • Blank and nil keys raise ArgumentError before any I/O, as with set. Previously nil raised NoMethodError, and a blank key failed inside the request, which closed the connection.

Not in scope: keys that need base64 are still limited by KeyManager truncating at 250 raw characters, while memcached caps the encoded key at 250 characters. That will be addressed separately.

Testing

  • New tests in test_namespace_and_key.rb call set_multi, then get and get_multi, on 1- and 2-server rings, with and without a namespace, for plain, space, tab, and non-ASCII keys. They also cover Symbol and Integer keys on both ring sizes. The three single-server cases fail on main.
  • These replace the two set_multi namespace tests from Fix set multi namespace on single server #47; the one named "multiple servers" ran on a single server without a namespace. The single-server concurrency test uses a namespace again, and its stale bug note is removed.
  • Full suite passes locally on Ruby 4.0.7 / memcached 1.6.45, excluding test_rack_session.rb, which does not load on Ruby 4.0 with or without this change (rack 2.2 requires the removed cgi/cookie).

The single-server set_multi path encoded keys in the protocol layer without the b flag and applied the namespace after encoding, so keys with whitespace or non-ASCII characters were stored under their base64 text, where get and get_multi never find them. Non-String keys raised NoMethodError.

PipelinedSetter now validates keys with the client's KeyManager (to_s, namespace, truncation) before the request, like PipelinedGetter and PipelinedDeleter, and write_multi_storage_req only encodes the key and sends b. The per-server KeyManager that existed only for this path is removed.
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.

1 participant