valkey - fix: clear() only removes keys in the exact namespace - #2147
Merged
Merged
Conversation
The KEYS pattern used by clear() when useSets is false was `namespace:<ns>*`, which also matched every namespace that starts with the same characters, so clearing `users` wiped `users-archive` too. Include the key separator in the pattern (`namespace:<ns>:*`) so only the adapter's own namespace matches, mirroring what iterator() already does. Adds regression tests for both useSets modes and documents the exact pattern in the README. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jt8UZWqPE62M8xJrbeSGws
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e5b819d5bc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2147 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 55 55
Lines 5281 5284 +3
Branches 857 855 -2
=========================================
+ Hits 5281 5284 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Codex review on #2147 pointed out that a namespace containing `*`, `?`, `[`, `]` or `\` was still treated as glob syntax by clear(), and the same line in iterator(), so `tenant*` could clear or iterate `tenant-prod`. Both now build their pattern through getKeyPattern(), which escapes those characters before appending the `:*` separator, with regression tests for both methods. A namespace that extends another with `:` (`users:archive` under `users`) is inherently ambiguous in the `namespace:<ns>:<key>` layout because keys may contain `:` too, so the README and JSDoc now state that limitation and point to `useSets: true` instead of claiming exact isolation. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jt8UZWqPE62M8xJrbeSGws
EmilBuszylo
pushed a commit
to EmilBuszylo/keyv
that referenced
this pull request
Sep 23, 2026
clear() and iterator() build their SCAN MATCH pattern from the raw namespace, so a namespace containing *, ?, [, ], or \ was treated as glob syntax and could match a different namespace (e.g. "tenant*" would also match "tenant-prod"). Route both through a new getKeyPattern() that escapes those characters before appending the :* separator, mirroring the fix already shipped for @keyv/valkey (jaredwray#2147). Also documents that a namespace extending another with ":" (e.g. "users:archive" under "users") still can't be told apart from a key containing ":" — use useSets: true for that separation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
EmilBuszylo
pushed a commit
to EmilBuszylo/keyv
that referenced
this pull request
Oct 1, 2026
clear() and iterator() build their SCAN MATCH pattern from the raw namespace, so a namespace containing *, ?, [, ], or \ was treated as glob syntax and could match a different namespace (e.g. "tenant*" would also match "tenant-prod"). Route both through a new getKeyPattern() that escapes those characters before appending the :* separator, mirroring the fix already shipped for @keyv/valkey (jaredwray#2147). Also documents that a namespace extending another with ":" (e.g. "users:archive" under "users") still can't be told apart from a key containing ":" — use useSets: true for that separation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
2 tasks done
jaredwray
added a commit
that referenced
this pull request
Oct 3, 2026
) clear() and iterator() passed `<namespace><keyPrefixSeparator>*` to SCAN MATCH without escaping it, so `*`, `?`, `[`, `]` and `\` in the namespace or separator were read as glob syntax. clear() on namespace `tenant*` deleted namespace `tenant-prod`'s keys and iterator() returned them, while a namespace such as `t[12]` or `a\b` never matched its own keys. v5 built the same pattern. Both now build the pattern through getKeyPattern(), which escapes those characters in the namespace and separator, as @keyv/valkey does since #2147. filterScannedKeys() also checks each SCAN result against the literal prefix, so clear() can't delete another namespace's keys whatever glob syntax the server supports. Adds regression tests for clear(), iterator() and a separator with glob characters, documents the literal matching and the separator limitation in the README, and notes the change in the v5-to-v6 migration guide and the keyv-migrate skill references. Claude-Session: https://claude.ai/code/session_01Wm1wtCGzQJEyYZdekgNVge Co-authored-by: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix.
Problem
When
useSetsisfalse(the default),clear()built itsKEYSpattern as${prefix}*, which expands tonamespace:<ns>*. That pattern also matches every namespace that merely starts with the same characters, so clearing namespaceusersalso deleted every key underusers-archive,users2, and so on.A second, related gap (raised by the Codex review on this PR): a namespace containing a glob metacharacter (
*,?,[,],\) was treated as pattern syntax by bothclear()anditerator(), sotenant*could clear or iteratetenant-prod.@keyv/redisis unaffected because it builds its pattern with the separator.Fix
storage/valkey/src/index.ts: new privategetKeyPattern()builds theKEYS/SCAN MATCHpattern asnamespace:<ns>:*with glob metacharacters in the prefix escaped, so only the adapter's own namespace matches literally.clear()anditerator()both use it. TheuseSets: truepath (SMEMBERS of the tracking set) is unchanged.storage/valkey/test/namespace.test.tsandtest/iterator.test.ts: regression tests that write into namespaceXand a siblingX<suffix>and assert the sibling survivesclear(), for bothuseSetsmodes, plus tests forclear()anditerator()with a namespace containing[a-z]?*\. TheuseSets: falsesibling test and both glob tests fail onmainand pass with the fix.storage/valkey/README.mdand theclear()/iterator()/getKeyPattern()JSDoc: document the exact pattern, that prefix-sharing namespaces are isolated, and one limitation that a pattern cannot remove: because:is the key separator and keys may contain:too, a namespace that extends another with:(users:archiveunderusers) is not distinguishable from keys containing:. The docs point touseSets: truefor that separation. Encoding or rejecting:would change the stored key layout for existing users, so it is out of scope for this fix.Verification
Ran
pnpm test:ciinstorage/valkeyagainst a local standalone server plus a 3-node cluster: lint clean, 150 tests passing (146 existing + 4 new), 100% line coverage.pnpm buildfor the package also passes. CI on the head commit is green on Node 22, 24 and 26, codecov (project and patch at 100%), bun, browser, and the security scans.Related
v5branch has the same pattern inpackages/valkey/src/index.ts(${this._getNamespace()}*) and will need a separate fix againstv5.@keyv/valkey-glideadapter in valkey-glide - feat: Add Valkey GLIDE storage adapter #2146 copies the same${prefix}*pattern and should get the same fix.🤖 Generated with Claude Code
https://claude.ai/code/session_01Jt8UZWqPE62M8xJrbeSGws