Skip to content

Conversation

murphyjacob4
Copy link
Contributor

When working on #2635 I errorneously duplicated the setSlotImportingStateInAllDbs call for successful imports. This resulted in us doubling the key count in the kvstore. This results in DBSIZE reporting an incorrect sum, and also causes BIT corruption that can eventually result in a crash.

The solution is:

  1. Only call setSlotImportingStateInAllDbs once (in our finishSlotMigrationJob function)
  2. Make setSlotImportingStateInAllDbs idempotent by checking if the delete from the kvstore importing hashtable is a no-op

This also fixes a bug where the number of importing keys is not lowered after the migration, but this is less critical since it is only used when resizing the dictionary on RDB load. However, it could result in un-loadable RDBs if the importing key count gets large enough.

Copy link

codecov bot commented Oct 18, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.63%. Comparing base (b4c93cc) to head (956f376).
⚠️ Report is 1 commits behind head on unstable.

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #2749      +/-   ##
============================================
+ Coverage     72.59%   72.63%   +0.04%     
============================================
  Files           128      128              
  Lines         71301    71300       -1     
============================================
+ Hits          51759    51791      +32     
+ Misses        19542    19509      -33     
Files with missing lines Coverage Δ
src/cluster_migrateslots.c 92.22% <ø> (+0.14%) ⬆️
src/kvstore.c 96.01% <100.00%> (ø)

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

Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

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

Looked at the code change but I didn't read the test cases carefully.

We should hurry and include this in 9.0.0, otherwise we're releasing with this known bug. Or should we already plan for 9.0.1?

@murphyjacob4
Copy link
Contributor Author

I agree - let's get it in to 9.0.0

@murphyjacob4 murphyjacob4 merged commit 2a914aa into valkey-io:unstable Oct 18, 2025
56 checks passed
@github-project-automation github-project-automation bot moved this to To be backported in Valkey 9.0 Oct 18, 2025
cherukum-Amazon pushed a commit to cherukum-Amazon/valkey that referenced this pull request Oct 19, 2025
…on (valkey-io#2749)

When working on valkey-io#2635 I errorneously duplicated the
setSlotImportingStateInAllDbs call for successful imports. This resulted
in us doubling the key count in the kvstore. This results in DBSIZE
reporting an incorrect sum, and also causes BIT corruption that can
eventually result in a crash.

The solution is:

1. Only call setSlotImportingStateInAllDbs once (in our
finishSlotMigrationJob function)
2. Make setSlotImportingStateInAllDbs idempotent by checking if the
delete from the kvstore importing hashtable is a no-op

This also fixes a bug where the number of importing keys is not lowered
after the migration, but this is less critical since it is only used
when resizing the dictionary on RDB load. However, it could result in
un-loadable RDBs if the importing key count gets large enough.

Signed-off-by: Jacob Murphy <[email protected]>
(cherry picked from commit 2a914aa)
cherukum-Amazon pushed a commit to cherukum-Amazon/valkey that referenced this pull request Oct 19, 2025
…on (valkey-io#2749)

When working on valkey-io#2635 I errorneously duplicated the
setSlotImportingStateInAllDbs call for successful imports. This resulted
in us doubling the key count in the kvstore. This results in DBSIZE
reporting an incorrect sum, and also causes BIT corruption that can
eventually result in a crash.

The solution is:

1. Only call setSlotImportingStateInAllDbs once (in our
finishSlotMigrationJob function)
2. Make setSlotImportingStateInAllDbs idempotent by checking if the
delete from the kvstore importing hashtable is a no-op

This also fixes a bug where the number of importing keys is not lowered
after the migration, but this is less critical since it is only used
when resizing the dictionary on RDB load. However, it could result in
un-loadable RDBs if the importing key count gets large enough.

Signed-off-by: Jacob Murphy <[email protected]>
(cherry picked from commit 2a914aa)
Signed-off-by: cherukum-amazon <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: To be backported

Development

Successfully merging this pull request may close these issues.

2 participants