Skip to content

Assertion check on null key for volatile fields based object#3897

Merged
ranshid merged 2 commits into
valkey-io:unstablefrom
ydsakshi:volatile_items_key_check
Jun 9, 2026
Merged

Assertion check on null key for volatile fields based object#3897
ranshid merged 2 commits into
valkey-io:unstablefrom
ydsakshi:volatile_items_key_check

Conversation

@ydsakshi

@ydsakshi ydsakshi commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes #3008

lets add assert checking that the object has a key in dbUntrackKeyWithVolatileItems and dbTrackKeyWithVolatileItems to be able to get a more explicit error in these cases

This is addressed in this PR.

@ydsakshi
add asserts on key check
ebdd2d3 
Signed-off-by: ydsakshi <ydsakshi023@gmail.com>
@coderabbitai

coderabbitai Bot commented Jun 2, 2026 •
edited
Loading

Copy link
Copy Markdown

Review Change Stack

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: 651878d9-1dd7-425d-929c-2ef043c66633

📥 Commits

Reviewing files that changed from the base of the PR and between 3cfaf57 and ebdd2d3.

📒 Files selected for processing (1)
  • src/db.c
📝 Walkthrough

Walkthrough

Two volatile-item tracking functions now validate preconditions: dbTrackKeyWithVolatileItems() and dbUntrackKeyWithVolatileItems() assert that tracked objects have non-NULL keys before computing dict indices and updating tracking state.

Changes

Volatile Item Tracking Preconditions

Layer / File(s) Summary
Volatile item tracking assertions
src/db.c		 dbTrackKeyWithVolatileItems() and dbUntrackKeyWithVolatileItems() add serverAssert(objectGetKey(o)) precondition checks to ensure tracked objects carry valid keys before proceeding with volatile-item state updates.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the primary change: adding assertion checks for null keys in volatile item tracking functions.
Description check ✅ Passed The description is related to the changeset, explaining the fix for issue #3008 and referencing the assertion checks being added.
Linked Issues check ✅ Passed The PR partially addresses linked issue #3008 by implementing assertion checks (#3008 goal 1), but does not implement the alternative approaches (#3008 goals 2-3).
Out of Scope Changes check ✅ Passed All changes are in-scope, limited to adding assertion checks in the two functions as specified in issue #3008.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

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

@ydsakshi ydsakshi mentioned this pull request Jun 2, 2026
Closed
@codecov

codecov Bot commented Jun 2, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.61%. Comparing base (f7b236d) to head (b86e00a).

Additional details and impacted files 
@@             Coverage Diff              @@
##           unstable    #3897      +/-   ##
============================================
- Coverage     76.68%   76.61%   -0.08%     
============================================
  Files           162      162              
  Lines         80731    80733       +2     
============================================
- Hits          61910    61852      -58     
- Misses        18821    18881      +60
Files with missing lines Coverage Δ
src/db.c 94.99% <100.00%> (+0.14%) ⬆️

... and 18 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.

@dvkashapov dvkashapov requested a review from ranshid June 2, 2026 19:50
ranshid
ranshid approved these changes Jun 3, 2026
View reviewed changes

@ranshid ranshid left a comment

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.

LGTM

@ranshid ranshid merged commit d9a80af into valkey-io:unstable Jun 9, 2026
62 checks passed
sarthakaggarwal97 added a commit to sarthakaggarwal97/valkey that referenced this pull request Jun 10, 2026
@sarthakaggarwal97
review: drop unrelated unstable-drift changes (valkey-io#3941, valkey…
5bc3dda 
…-io#3942, valkey-io#3897)

These came in via the unstable merge in valkey-io#3853 but are unrelated to
replication compression. Restore cluster_legacy.c, db.c, and
failover.tcl to the rio-pr base so this review branch shows only the
replication-compression delta.

Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ranshid ranshid ranshid approved these changes

Assignees

@ydsakshi ydsakshi

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Rethink assumptions in dbUntrack/TrackKeyWithVolatileItems

2 participants

@ydsakshi @ranshid