hashtable: add bounds check in hashtableNext iterator

[uriyage]

Prevent iteration beyond hashtable bounds by checking if iterator is already at the end before proceeding with the main iteration loop. This adds defensive bounds checking for table index and bucket index.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.03%. Comparing base (a47e8fa) to head (c57878c).
⚠️ Report is 22 commits behind head on unstable.

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #2611      +/-   ##
============================================
- Coverage     72.24%   72.03%   -0.22%     
============================================
  Files           127      128       +1     
  Lines         70820    71041     +221     
============================================
+ Hits          51167    51175       +8     
- Misses        19653    19866     +213     

Files with missing lines	Coverage Δ
src/hashtable.c	89.35% <100.00%> (+0.04%)	⬆️

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

[zuiderkwast]

Thanks Uri!

Note that this is just for making it safe to call hashtableNext again when it has already returned false. It's not really needed, but it makes it easier to detect bad usage, or is it for convenience?

Isn't calling hashtableNext() after the end a bug that we want to detect? Why not do an assert instead of graceful return then?

[ranshid]

@uriyage let's also add unittest for this?

[uriyage]

@uriyage let's also add unittest for this?

Done

[ranshid]

LGTM

[zuiderkwast]

Many times I've tried to reject adding dependencies from the low-level datastructures to the whole of the server globals and everything. Dependencies should be in the other direction, from high level to lower level components.

[madolson]

I somewhat agree with Viktor. We can add a debugAssert to severassert.h if we want, that is just a no-op.

I was also dubious of this PR, since it's just adding a defensive check. @uriyage @ranshid Are we still moving forward with this?

[zuiderkwast]

I somewhat agree with Viktor. We can add a debugAssert to severassert.h if we want, that is just a no-op.

We can just use assert instead. The check in the asserts are very light-weight, not even heavier than checking if the debugging is enabled.

Debug assert depends on a server config. That would prevent usage of this hashtable in non-server code (valkey-cli, valkey-benchmark).

[zuiderkwast]

@madolson According to a commment above, Uri explained why:

We have a use case where we don't save the iterator state and attempt to run it again.

I suppose we could wait for this use case to be upstreamed and then include this change at the same time.

[madolson]

Couldn't they then just save the state?

I agree, I feel like I would rather close this and re-open if that use case becomes public. I'm not sure what that case is.