fix: Prevent EXISTS from incrementing keyspace hit/miss stats

[yulazariy]

Add LOOKUP_NOSTATS flag to lookupKeyReadWithFlags in existsCommand so that EXISTS does not affect keyspace_hits or keyspace_misses counters. These stats are meant for cache hit/miss tracking and EXISTS is a key existence check, not a cache access.

[coderabbitai]

📝 Walkthrough

Walkthrough

existsCommand in src/db.c adds LOOKUP_NOSTATS to its lookupKeyReadWithFlags call, preventing keyspace hit/miss counter increments. A new test in tests/unit/keyspace.tcl snapshots the counters, runs EXISTS on existing and missing keys, and asserts the counters are unchanged.

EXISTS keyspace stats suppression

Layer / File(s)	Summary
Add LOOKUP_NOSTATS to existsCommand and verify with test src/db.c, tests/unit/keyspace.tcl	existsCommand now passes LOOKUP_NOTOUCH | LOOKUP_NOSTATS so keyspace hit/miss counters are not updated. The new test snapshots keyspace_hits and keyspace_misses, calls EXISTS for both an existing and a non-existing key, and asserts both counters remain unchanged.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately and concisely describes the main change: preventing EXISTS from incrementing keyspace hit/miss stats.
Description check	✅ Passed	The description clearly explains the change (adding LOOKUP_NOSTATS flag), the affected function, and the rationale for the change.
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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 1

🤖 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.

Inline comments:
In `@tests/unit/keyspace.tcl`:
- 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.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 8212c92c-acf2-40a1-9bae-102e6dddc662

📥 Commits

Reviewing files that changed from the base of the PR and between 699c64e and 33b8aa6.

📒 Files selected for processing (2)

• src/db.c
• tests/unit/keyspace.tcl

[coderabbitai]

⚠️ 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.

[ranshid]

Thank you @yulazariy IMO this makes sense. I do think this is makes sense, but it is also a behavioral change (even if never documented)
I will flag it as "major-decision" and ask @valkey-io/core-team to +/-1.
(BTW - currently set major-decision-needed". once another TSC member approve, will make it major-decision-pending-auto-approve)

[yulazariy]

cool thanks @ranshid

Also looks like there is a (not related) flakey test https://github.com/valkey-io/valkey/actions/runs/27496365556/job/81271055172?pr=3986

[zuiderkwast]

@valkey-review-bot Is this a backward compatible change according to semantic versioning?

[valkey-review-bot]

On the backward-compatibility question (@zuiderkwast): this is a behavioral change to a public, observable interface, not an API-breaking one.

• No command surface changes: EXISTS syntax, arity, and reply are untouched, so nothing a client parses breaks.
• What changes is the semantics of the keyspace_hits / keyspace_misses fields in INFO stats (src/server.c:6517-6518), which monitoring and cache-efficiency tooling consume. After this PR, EXISTS no longer contributes to either counter, so any dashboard/alert that today includes EXISTS traffic in its hit/miss ratio will shift. The per-command contribution was never documented, but the counters themselves are a stable public metric.

So: API-compatible, but a metric-semantics change. Under semver this fits a minor release (or a patch with an explicit release note), not a silent patch — consistent with @ranshid flagging it as a major-decision. One thing worth settling before merge: the change leaves EXISTS inconsistent with its sibling read-introspection commands — TYPE (src/db.c:1420, LOOKUP_NOTOUCH only) and OBJECT (src/object.c:1687, LOOKUP_NOTOUCH | LOOKUP_NONOTIFY) both still increment hits/misses. If the rationale is "existence/introspection checks aren't cache accesses," it applies equally to those; if it's EXISTS-specific, that's worth stating.

One inline note on a side effect of the chosen flag.

[valkey-review-bot]

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.

[zuiderkwast]

@valkey-review-bot Whether the behavior change breaks the documented behavior depends on whether this behavior is publicly documented for this command and these stats, right? The public documentation is in the valkey-doc repo.

[valkey-review-bot]

On the documentation question (@zuiderkwast): I can't inspect the valkey-doc repo from here, so I can't confirm what it states about EXISTS or about keyspace_hits/keyspace_misses — that check has to happen against valkey-doc directly. What I can confirm from this repo:

• The in-repo command metadata for EXISTS (src/commands/exists.json) documents only summary, complexity, and reply shape. It says nothing about keyspace hit/miss accounting, so there is no in-repo documented contract this change contradicts.
• There is no test or code in the tree that asserts EXISTS contributes to keyspace_hits/keyspace_misses (no match for such a coupling), so there is no in-repo regression signal either.

So whether this "breaks documented behavior" reduces entirely to what valkey-doc says about these two INFO fields and whether per-command contribution is described there. If valkey-doc only describes the counters generically (cache hit/miss tracking) without enumerating which commands feed them, this is a semantics shift but not a contradiction of a written contract; if it explicitly lists EXISTS as a contributor, it would be.

One code-level note that bears on framing this purely as a "stats" change: with LOOKUP_NOSTATS alone, EXISTS on a missing key still fires the keymiss keyspace notification (gated separately on LOOKUP_NONOTIFY at src/db.c:120-121), so the counter and the notification — previously in lockstep for this command — now diverge. That is already noted inline at src/db.c:904, and the divergence from the TYPE/OBJECT introspection siblings was raised earlier in the thread; no new inline comment needed here.