Skip to content

fix: Prevent EXISTS from incrementing keyspace hit/miss stats #3986

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
yulazariy wants to merge 1 commit into
base: unstable
Choose a base branch
from
+16 −1
Open

fix: Prevent EXISTS from incrementing keyspace hit/miss stats #3986

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/db.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -901,7 +901,7 @@ void existsCommand(client *c) {
int j;

for (j = 1; j < c->argc; j++) {
if (lookupKeyReadWithFlags(c->db, c->argv[j], LOOKUP_NOTOUCH)) count++;
if (lookupKeyReadWithFlags(c->db, c->argv[j], LOOKUP_NOTOUCH | LOOKUP_NOSTATS)) count++;

@valkey-review-bot valkey-review-bot Bot Jun 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LOOKUP_NOSTATS suppresses the keyspace_misses counter, but the keymiss keyspace notification is gated separately on LOOKUP_NONOTIFY (src/db.c:120-121), which is not passed here. So after this change EXISTS on a missing key still fires a NOTIFY_KEY_MISS (keymiss) event while no longer incrementing keyspace_misses — the two were previously in lockstep for this command.

If the intent is "EXISTS is not a cache access," the notification arguably should be suppressed too, for consistency with the stats:

Suggested change
if (lookupKeyReadWithFlags(c->db, c->argv[j], LOOKUP_NOTOUCH | LOOKUP_NOSTATS)) count++;
if (lookupKeyReadWithFlags(c->db, c->argv[j], LOOKUP_NOTOUCH | LOOKUP_NOSTATS | LOOKUP_NONOTIFY)) count++;

If firing keymiss for EXISTS is intentional (some clients rely on it for key-tracking/invalidation), then leaving it as-is is fine — but worth confirming, since the divergence is easy to miss.

@ranshid ranshid Jun 15, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@valkey-review-bot good catch about the LOOKUP_NONOTIFY. what other implications will there be if we add that one as well?

@valkey-review-bot valkey-review-bot Bot Jun 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On what adding LOOKUP_NONOTIFY would change here — the keymiss event has exactly two consumers, and tracking is not one of them:

  • Keyspace pub/sub: subscribers of __keyevent@<db>__:keymiss (i.e. operators running with m in notify-keyspace-events) would stop seeing an event when EXISTS probes a missing key. This branch is config-gated at src/notify.c:131-157.
  • Modules: moduleNotifyKeyspaceEvent (src/notify.c:124) fires unconditionally, so any module subscribed via VALKEYMODULE_NOTIFY_KEY_MISS would stop getting the callback for EXISTS misses.
  • Client-side caching / invalidation is unaffected. Key tracking is driven by the CMD_READONLY flag in src/server.c:4083-4091 (trackingRememberKeys), independent of both keyspace_misses and the keymiss notification — so neither this PR nor adding LOOKUP_NONOTIFY changes what tracking-enabled clients get invalidated on.

So the only behavior at stake is the keymiss keyspace event / module notification, not invalidation. If the intent is "EXISTS is a pure existence probe that produces no observable side effects," the codebase already has a name for that exact combination:

Suggested change
if (lookupKeyReadWithFlags(c->db, c->argv[j], LOOKUP_NOTOUCH | LOOKUP_NOSTATS)) count++;
if (lookupKeyReadWithFlags(c->db, c->argv[j], LOOKUP_NOEFFECTS)) count++;

LOOKUP_NOEFFECTS is defined at src/server.h:3753 as LOOKUP_NONOTIFY | LOOKUP_NOSTATS | LOOKUP_NOTOUCH | LOOKUP_NOEXPIRE. Using it would keep stats and the notification in lockstep (both suppressed) and match the documented "avoid any effects from fetching the key" intent. If instead you want EXISTS to keep emitting keymiss (e.g. to preserve the module/pub-sub signal that a probe occurred) while only dropping the counter, the current LOOKUP_NOTOUCH | LOOKUP_NOSTATS is fine — but that intentional split is worth a one-line comment, since the next reader will see stats and notification diverge and assume it was an oversight.

}
addReplyLongLong(c, count);
}
Expand Down
15 changes: 15 additions & 0 deletions tests/unit/keyspace.tcl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,21 @@ start_server {tags {"keyspace"}} {
append res [r exists newkey]
} {10}

test {EXISTS does not update keyspace hits or misses} {
r set existskey value
set info [r info stats]
set hits_before [getInfoProperty $info keyspace_hits]
set misses_before [getInfoProperty $info keyspace_misses]
r exists existskey
r exists nonexistingkey

@coderabbitai coderabbitai Bot Jun 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Make the miss-path assertion deterministic.

At Line 76, nonexistingkey is assumed absent; if prior test state leaks, this can become a hit and stop exercising the miss path. Explicitly delete/assert absence before the EXISTS call.

Suggested patch 
     test {EXISTS does not update keyspace hits or misses} {
         r set existskey value
+        r del nonexistingkey
         set info [r info stats]
         set hits_before [getInfoProperty $info keyspace_hits]
         set misses_before [getInfoProperty $info keyspace_misses]
         r exists existskey
         r exists nonexistingkey
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
r exists nonexistingkey
test {EXISTS does not update keyspace hits or misses} {
r set existskey value
r del nonexistingkey
set info [r info stats]
set hits_before [getInfoProperty $info keyspace_hits]
set misses_before [getInfoProperty $info keyspace_misses]
r exists existskey
r exists nonexistingkey
🤖 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/keyspace.tcl` at line 76, The test at line 76 assumes that
nonexistingkey is absent when calling r exists nonexistingkey, but does not
explicitly ensure this beforehand. This can cause test failures if prior test
state leaks and the key actually exists, preventing the miss path from being
exercised. Before the EXISTS call on nonexistingkey, explicitly delete the key
using r del nonexistingkey or assert its absence to guarantee deterministic
behavior and ensure the miss path is always tested regardless of prior test
state.

set info [r info stats]
set hits_after [getInfoProperty $info keyspace_hits]
set misses_after [getInfoProperty $info keyspace_misses]
assert_equal $hits_before $hits_after
assert_equal $misses_before $misses_after
r del existskey
}

test {Zero length value in key. SET/GET/EXISTS} {
r set emptykey {}
set res [r get emptykey]
Expand Down
Loading