fix: restore KvEntry Eq, make contains_key expiry-aware, fix clear_expired crashes

[uzyn]

Summary

Address two code-review findings on the SparKV<V> generic-value release, plus two crash bugs discovered while analyzing the expiry machinery.

• Item 3 — KvEntry lost PartialEq/Eq. Restore the derives (public-API regression). On a generic struct they generate bounded impls, so V = String regains them and any V: Eq gets them conditionally.
• Item 4 — contains_key()/len() count expired-but-unswept entries. An exact expiry-aware len() cannot be O(1) under lazy expiry, so contains_key is made expiry-aware (a free, exact O(1) &self fix) while len()/is_empty() stay O(1) physical counts and are documented.
• Bug fixes — clear_expired(). A panic on a deleted key's stale ExpEntry, and a stack overflow under the default config from re-entrant auto-clear.

Requirements

• KvEntry derives Debug, Clone, PartialEq, Eq again + test_eq.
• contains_key(key) returns false for an expired-but-unswept entry, true for a live one; consistent with get().
• len() / is_empty() remain O(1) physical counts (unchanged behavior), with Rustdoc explaining the physical-vs-logical distinction and pointing to get()/clear_expired().
• Internal capacity accounting is unchanged: ensure_capacity_ignore_key uses the physical self.data.contains_key(key) (not the now expiry-aware public contains_key), so set() overwrite/capacity behavior and existing capacity tests are preserved.
• clear_expired() no longer panics on a stale/deleted key (guard the get with if let Some(..)).
• clear_expired() no longer recurses/overflows under the default config (remove via self.data.remove(&key) instead of self.delete(&key)).
• Regression tests for: expiry-aware contains_key; the delete-then-expire panic path; the default-config recursion path.
• cargo fmt --check, cargo test, cargo clippy -- -D warnings all pass.

Out of scope

• Making len()/is_empty() exact/expiry-aware (would require O(n) scan or a breaking &mut self — explicitly declined in favor of O(1)).
• The lazy-expiry model, the expiries heap design, auto_clear_expired.
• The other review items (1, 2, 5, …).
• The cosmetic boundary mismatch between get_item (expired_at > now) and ExpEntry::is_expired (expired_at < now) at the exact == instant — noted, not fixed.

Technical approach

• Item 3: src/kventry.rs — #[derive(Debug, Clone, PartialEq, Eq)]; test_eq.
• Item 4 (src/lib.rs): contains_key(&self, key) -> self.get_item(key).is_some() (O(1), expiry-aware). ensure_capacity_ignore_key uses physical self.data.contains_key(key) to keep capacity/overwrite semantics identical. len/is_empty bodies unchanged; Rustdoc added to all three accessors.
• Bug fixes (src/lib.rs clear_expired): within the is_expired() branch, the self.data.get(..).unwrap() + self.delete(..) is replaced with a guarded, non-recursive form — if let Some(kv_entry) = self.data.get(..) plus self.data.remove(..) instead of self.delete(..). delete() only mutated data (the heap pop already happens here), so cleanup semantics are preserved while the unwrap panic and the re-entrant auto-clear recursion are both removed.

Test plan

• Kept test_eq (Item 3).
• Added test_contains_key_is_expiry_aware: contains_key is false for an expired-but-unswept key and true for a live one (with auto_clear_expired = false), agreeing with get() while the entry stays physically counted.
• Added test_clear_expired_does_not_panic_on_deleted_key: delete-then-expire-then-clear_expired() does not panic and cleans up correctly.
• Added test_clear_expired_does_not_recurse_under_default_config: default-config set -> expire -> set again does not overflow; expired entry cleared, live one remains.
• Existing capacity/overwrite/clear_expired tests still pass.
• Results: all 28 tests pass; cargo fmt --check, cargo clippy --all-targets -- -D warnings clean. All three new tests were confirmed to fail (reproduce the bugs) against the unfixed code before the fixes were applied.

Notes

• Item 3 restores a public-API regression (KvEntry PartialEq/Eq).
• Item 4: exact expiry-aware len() is provably not O(1) under lazy expiry; len()/is_empty() stay O(1) physical + documented (matches Redis/Caffeine/Guava/moka/go-cache), contains_key is the free exact O(1) fix.
• The two clear_expired() crash bugs (unwrap panic; default-config stack overflow) were discovered during analysis and reproduced before fixing.

[uzyn]

Standalone Review — PR #3: restore KvEntry Eq, expiry-aware contains_key, fix clear_expired crashes

Reviewed against the PR description as scope. No PRD/sprint plan (standalone mode). I independently verified every claim, including reverting the fix to confirm the new tests reproduce the bugs.

Changes Overview

Four changes to the sparkv crate: (1) restore #[derive(PartialEq, Eq)] on KvEntry<V> — a public-API regression from the generic-value release; (2) make contains_key expiry-aware by delegating to get_item().is_some() (O(1), &self), with new Rustdoc on len/is_empty/contains_key documenting the physical-vs-logical distinction; (3) fix a .unwrap() panic in clear_expired on a deleted key's stale ExpEntry by guarding with if let Some; (4) fix a stack-overflow from re-entrant auto-clear by removing via self.data.remove instead of self.delete.

Scope Alignment

Fully met, no scope creep. Every checkbox in the PR's Requirements list is genuinely satisfied:

• Item 3: KvEntry derives Debug, Clone, PartialEq, Eq again (src/kventry.rs:1); test_eq added.
• Item 4: contains_key is now self.get_item(key).is_some() (src/lib.rs:71-73) — expiry-aware and O(1), consistent with get().
• len()/is_empty() bodies unchanged, O(1) physical, with accurate Rustdoc. This is the explicit, defensible maintainer decision (matches Redis DBSIZE, Caffeine, Guava, moka, go-cache) — correct not to "fix."
• ensure_capacity_ignore_key correctly switched to physical self.data.contains_key(key) (src/lib.rs:117), preserving overwrite/capacity semantics. Verified: test_set_should_fail_if_capacity_exceeded (overwrite-does-not-err path) and test_clear_expired_with_overwritten_key still pass.

The get_item (> now) vs is_expired (< now) boundary at the exact == instant is explicitly noted as out-of-scope. Agreed — it is pre-existing, cosmetic, and unreachable in practice.

Potential Bugs

None found. The two crash fixes are correct:

• Panic fix (src/lib.rs:82-89): if let Some(kv_entry) = self.data.get(...) correctly handles the case where a key was deleted but its stale ExpEntry remains on the heap (delete intentionally does not pop the heap). The self.expiries.pop() still runs unconditionally, so stale entries are drained either way.
• Recursion fix (src/lib.rs:87): self.data.remove instead of self.delete breaks the clear_expired → delete → clear_expired_if_auto → clear_expired cycle that overflowed the stack under the default config (auto_clear_expired = true). delete's only data-mutating effect was self.data.remove, and the heap pop already happens in clear_expired, so cleanup semantics are preserved exactly.

Borrow-checker correctness (verified): the immutable borrow from self.data.get and the mutable borrow from self.data.remove do not conflict because exp_item is an owned clone (peek().cloned()), so &exp_item.key does not borrow self, and NLL ends the get borrow at the last use of kv_entry before remove. Compiles clean under clippy -D warnings.

Test Coverage

Strong. I reverted the fix to the original buggy code and confirmed the new tests actually catch the bugs (not happy-path theater):

• test_clear_expired_does_not_panic_on_deleted_key → FAILED on the buggy .unwrap() (panic), passes on the fix.
• test_clear_expired_does_not_recurse_under_default_config → stack overflow / SIGABRT on the buggy recursion, passes on the fix.
• test_contains_key_is_expiry_aware exercises all three states: expired-but-unswept (agrees with get, still physically counted via len()==2), live, and non-existent.
• test_eq covers both equal (clone) and unequal (mutated value) paths.

Full suite: 28/28 pass, cargo clippy --all-targets -- -D warnings clean, cargo fmt --check clean.

Security Issues

None. No new inputs, dependencies, unsafe code, or data exposure. The changes strictly remove two crash vectors.

Code Quality

Clean. The inline comments at src/lib.rs:87 and :115-116 explain the two non-obvious decisions (why data.remove not delete; why physical contains_key for capacity) exactly where a future reader needs them. Rustdoc on the three accessors is accurate and links correctly.

Summary and Recommended Actions

• Overall verdict: Ready to merge
• Blockers: none
• Non-blockers: none
• Nice-to-haves: none

This is a tight, well-scoped fix. Both crash bugs are real and now provably covered by regression tests; the API-regression and expiry-awareness items are correctly resolved; the deliberate O(1)-len decision is sound and documented. Verification matches every claim in the PR description.

Recommended merge commit message:

fix: restore KvEntry Eq, make contains_key expiry-aware, fix clear_expired crashes

Restore KvEntry's PartialEq/Eq derives (public-API regression on the generic-value
release). Make contains_key expiry-aware via get_item so it agrees with get(), while
keeping len()/is_empty() as documented O(1) physical counts; ensure_capacity_ignore_key
uses physical data.contains_key to keep capacity/overwrite semantics identical.

Fix two clear_expired() crashes: guard data.get() with `if let Some` to avoid unwrapping
a deleted key's stale ExpEntry (panic), and remove via data.remove() instead of
self.delete() to avoid re-entrant auto-clear recursion that overflowed the stack under
the default config. Regression tests added for all three.