Skip NOT_KEY commands in client tracking by nmvk · Pull Request #3699 · valkey-io/valkey

nmvk · 2026-05-13T17:04:46Z

While reviewing #3675 I found a pre-existing NOT_KEY inconsistency in client tracking.

CLUSTERSCAN is skipped by COMMAND GETKEYS/ACL through the key spec path, but tracking still uses getKeysFromCommand(). Updated to use getKeysFromCommandWithSpecs

raghav$ printf 'client tracking on\ninfo stats\nclusterscan 0-{06S}-0\ninfo stats\n'   | /tmp/valkey-unstable-single/src/valkey-cli -p 7001 --raw   | grep -E '^(OK|tracking_total_keys:|0$)'
OK
tracking_total_keys:2
0
tracking_total_keys:2

This affects today CLUSTERSCAN because it is RO, SPUBLISH/SSUBSCRIBE/SUNSUBSCRIBE using NOT_KEY are not RO.

While reviewing valkey-io#3675 I found a pre-existing NOT_KEY inconsistency in client tracking: CLUSTERSCAN is skipped by COMMAND GETKEYS/ACL through the key-spec path, but tracking still uses the legacy getKeysFromCommand() Signed-off-by: nmvk <r@nmvk.com>

coderabbitai · 2026-05-13T17:05:06Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 21fda79d-8db4-44b2-9151-e638a031f02d

📥 Commits

Reviewing files that changed from the base of the PR and between a14b09c and ac1a03d.

📒 Files selected for processing (1)

src/tracking.c

🚧 Files skipped from review as they are similar to previous changes (1)

src/tracking.c

📝 Walkthrough

Walkthrough

trackingRememberKeys() now uses getKeysFromCommandWithSpecs(..., GET_KEYSPEC_DEFAULT) to extract keys for tracking instead of getKeysFromCommand(). A new unit test verifies that CLUSTERSCAN does not trigger client tracking.

Changes

Client Tracking Key Extraction API Update

Layer / File(s)	Summary
Key extraction API upgrade `src/tracking.c`	`trackingRememberKeys()` replaces `getKeysFromCommand()` with `getKeysFromCommandWithSpecs(..., GET_KEYSPEC_DEFAULT)` to compute `numkeys` and populate `getKeysResult` for tracking.
CLUSTERSCAN tracking exclusion test `tests/unit/cluster/clusterscan.tcl`	Adds a test that enables client tracking, asserts `tracking_total_keys` remains `0` before and after `clusterscan` with a valid cursor, then disables client tracking.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: updating client tracking to skip NOT_KEY commands by using getKeysFromCommandWithSpecs instead of the legacy getKeysFromCommand.
Description check	✅ Passed	The description is directly related to the changeset, explaining the pre-existing inconsistency found during review, the specific fix applied, and providing a concrete example demonstrating the issue with CLUSTERSCAN.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

🧹 Nitpick comments (1)

tests/unit/cluster/clusterscan.tcl (1)

43-51: 💤 Low value

Consider adding descriptive error messages to assertions.

The test correctly validates that CLUSTERSCAN does not modify tracking_total_keys. However, the assertions would benefit from descriptive error messages to aid debugging if they fail.

✨ Suggested enhancement

 test "CLUSTERSCAN cursor is not tracked by client tracking" {
     R 0 client tracking on
-    assert_equal 0 [s 0 tracking_total_keys]
+    assert_equal 0 [s 0 tracking_total_keys] "Expected tracking_total_keys to be 0 initially"

     R 0 clusterscan "0-{06S}-0"
-    assert_equal 0 [s 0 tracking_total_keys]
+    assert_equal 0 [s 0 tracking_total_keys] "Expected tracking_total_keys to remain 0 after CLUSTERSCAN"

     R 0 client tracking off
 }

As per coding guidelines, tests should use clear assertions with meaningful error messages.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/unit/cluster/clusterscan.tcl` around lines 43 - 51, The assertions in
the test "CLUSTERSCAN cursor is not tracked by client tracking" lack descriptive
error messages; update the two assert_equal calls that check [s 0
tracking_total_keys] to include clear failure messages (e.g., indicating
expected 0 tracking_total_keys before and after R 0 clusterscan "0-{06S}-0"), so
if tracking_total_keys changes the assertion output names the test, the
expectation, and whether it was before or after the clusterscan call; modify the
assert_equal invocations in that test block accordingly.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@tests/unit/cluster/clusterscan.tcl`:
- Around line 43-51: The assertions in the test "CLUSTERSCAN cursor is not
tracked by client tracking" lack descriptive error messages; update the two
assert_equal calls that check [s 0 tracking_total_keys] to include clear failure
messages (e.g., indicating expected 0 tracking_total_keys before and after R 0
clusterscan "0-{06S}-0"), so if tracking_total_keys changes the assertion output
names the test, the expectation, and whether it was before or after the
clusterscan call; modify the assert_equal invocations in that test block
accordingly.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: f1821092-2051-4bc6-aeed-2bb22f38c8b7

📥 Commits

Reviewing files that changed from the base of the PR and between 5b167f4 and a14b09c.

📒 Files selected for processing (2)

src/tracking.c
tests/unit/cluster/clusterscan.tcl

codecov · 2026-05-13T17:31:21Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.64%. Comparing base (5b167f4) to head (ac1a03d).
⚠️ Report is 37 commits behind head on unstable.

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #3699      +/-   ##
============================================
- Coverage     76.71%   76.64%   -0.08%     
============================================
  Files           162      162              
  Lines         80662    80717      +55     
============================================
- Hits          61881    61863      -18     
- Misses        18781    18854      +73

Files with missing lines	Coverage Δ
src/tracking.c	`99.34% <100.00%> (ø)`

... and 34 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

enjoy-binbin

Thanks, LGTM, i actually noticed that too, but did not bother to go deep. You've checked everywhere, and this is the last one, right?

nmvk · 2026-05-14T04:46:28Z

Thanks @enjoy-binbin I believe there should not be anything else for CLUSTERSCAN. I looked at old discussion when the NOT_KEY was added it was more specifically around handling the ACL.

VALKEYMODULE_CMD_KEY_NOT_KEY does not seem to be consistently skipped by ACL/tracking when used via ValkeyModule_KeyAtPosWithFlags this is an additional observation.

Co-authored-by: Madelyn Olson <madelyneolson@gmail.com> Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>

…3675) No bug exists — all NOT_KEY commands (CLUSTERSCAN, SSUBSCRIBE, SPUBLISH, SUNSUBSCRIBE) have NULL getkeys_proc, so doesCommandHaveKeys returns 0 and ACL correctly skips key checks. This PR makes the handling more explicit and defensive for future commands that may combine getkeys_proc with NOT_KEY key-specs. Changes: - Refactor doesCommandHaveKeys() from a one-line ternary into clearer if-else branches. Add an explicit check: if a command has getkeys_proc but all its key-specs are NOT_KEY, treat it as having no real keys. - Add a defensive NOT_KEY check in ACLSelectorCheckKey() to skip key-pattern ACL validation for NOT_KEY entries, consistent with the existing skip in getKeysUsingKeySpecs(). - Add tests: verify COMMAND GETKEYS/GETKEYSANDFLAGS report "no key arguments" for NOT_KEY commands; verify restricted ACL users can run CLUSTERSCAN without key-permission errors. Related: #2934, #3699 Signed-off-by: Binbin <binloveplay1314@qq.com>

coderabbitai Bot reviewed May 13, 2026

View reviewed changes

github-actions Bot assigned nmvk May 13, 2026

enjoy-binbin approved these changes May 14, 2026

View reviewed changes

enjoy-binbin added this to Valkey 9.1 May 14, 2026

enjoy-binbin requested a review from madolson May 15, 2026 09:03

madolson approved these changes May 18, 2026

View reviewed changes

madolson removed this from Valkey 9.1 May 18, 2026

madolson added this to Valkey 10 May 18, 2026

madolson reviewed Jun 1, 2026

View reviewed changes

Comment thread src/tracking.c Outdated

Apply suggestions from code review

ac1a03d

Co-authored-by: Madelyn Olson <madelyneolson@gmail.com> Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>

enjoy-binbin mentioned this pull request Jun 25, 2026

Tighten NOT_KEY handling and add test coverage for NOT_KEY commands #3675

Merged

enjoy-binbin merged commit c4cfa0f into valkey-io:unstable Jun 25, 2026
63 checks passed

github-project-automation Bot moved this to Merged in Valkey 10 Jun 25, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Skip NOT_KEY commands in client tracking#3699

Skip NOT_KEY commands in client tracking#3699
enjoy-binbin merged 2 commits into
valkey-io:unstablefrom
nmvk:tracking

nmvk commented May 13, 2026

Uh oh!

coderabbitai Bot commented May 13, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Uh oh!

coderabbitai Bot left a comment

Uh oh!

codecov Bot commented May 13, 2026 •

edited

Loading

Uh oh!

enjoy-binbin left a comment

Uh oh!

nmvk commented May 14, 2026

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

nmvk commented May 13, 2026

Uh oh!

coderabbitai Bot commented May 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented May 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

enjoy-binbin left a comment

Choose a reason for hiding this comment

Uh oh!

nmvk commented May 14, 2026

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coderabbitai Bot commented May 13, 2026 •

edited

Loading

codecov Bot commented May 13, 2026 •

edited

Loading