fix: prevent clear_expired infinite recursion and stale-entry panic

[uzyn]

Summary

SparKV::clear_expired had two latent defects that surface under normal, default usage. Both reported feedback items were confirmed valid. This PR fixes both with minimal, targeted changes plus regression tests.

• Item 1 (infinite recursion): clear_expired called self.delete(&key) for a live expired entry; delete calls clear_expired_if_auto(), which under the default auto_clear_expired == true re-enters clear_expired() before the heap entry is popped and before the data entry is removed — so it peeks the same live expired entry and recurses unbounded into a stack overflow.
• Item 2 (panic on stale heap entry): clear_expired did self.data.get(&key).unwrap(). Since delete intentionally leaves expiry entries in the heap, a key deleted before its expiry yields a stale heap entry; when cleanup runs after that expiry, data.get returns None and .unwrap() panics.

Requirements

• clear_expired must not recurse via delete when auto_clear_expired is true (no stack overflow). Uses an internal removal path that skips the auto-clear hook.
• clear_expired must not panic on a stale heap entry whose key is absent from data; it treats it as stale and just pops it.
• The public behavior of delete is unchanged (still triggers auto-clear, still returns the removed value, still leaves the heap entry in place).
• Existing behaviors preserved: overwritten-key entries still skipped (not counted, popped); clear_expired return count semantics unchanged; heap is drained of leading expired entries.
• Regression tests for both bugs that fail on current code and pass after the fix.

Out of scope

• Eager/lazy removal of stale heap entries on delete (the heap is intentionally not pruned on delete for cost reasons — that design is kept).
• Any change to Config, TTL semantics, capacity, or the generic-value API.
• Broader concurrency / thread-safety work.

Technical approach

• Added a private delete_internal(&mut self, key: &str) -> Option<V> that only does self.data.remove(key).map(|item| item.value) — no auto-clear hook.
• delete now calls self.clear_expired_if_auto(); self.delete_internal(key) — identical public behavior.
• In clear_expired, replaced self.delete(&exp_item.key) with self.delete_internal(&exp_item.key) (fixes Item 1), and replaced self.data.get(&exp_item.key).unwrap() with an if let Some(kv_entry) = ... guard so a missing entry falls through to self.expiries.pop() (fixes Item 2). The non-matching expired_at (overwritten key) case keeps current behavior: not counted, popped.

Test plan

Added three tests to the tests module in src/lib.rs (matching existing style, std::thread::sleep for TTL expiry):

• test_clear_expired_no_recursion_with_auto_clear: default config, short-TTL live key, sleep past it, then set another key. Completes without stack overflow; expired key gone, new key present, counts correct. (Item 1)
• test_clear_expired_skips_stale_heap_entry_no_panic: set a key with a TTL, delete it before expiry, sleep past TTL, call clear_expired(). No panic; returns 0 cleared; stale heap entry popped. (Item 2)
• test_clear_expired_stale_entry_and_live_expired_with_auto_clear: combined deleted (stale) key + separate live expired key under default auto-clear; no panic, no recursion, count correct.

Verified the two regression tests fail on the unfixed code (Item 1 stack-overflows/aborts, Item 2 panics on unwrap), then pass after the fix. Results: all 27 tests pass, clippy clean (cargo clippy -- -D warnings), fmt clean (cargo fmt --check).

Notes

Both fixes ship in one PR since both defects live in clear_expired. Base branch is main.

[uzyn]

Standalone Review — PR #2: fix clear_expired infinite recursion and stale-entry panic

Reviewed against the PR description (no PRD/sprint plan; standalone mode). I read the full src/lib.rs, the supporting types (ExpEntry, KvEntry, Config), and independently reconstructed the pre-fix code to verify the regression tests actually exercise the bug paths.

Changes Overview

The PR fixes two latent defects in SparKV::clear_expired. It introduces a private delete_internal that removes the data entry without firing the auto-clear hook, and rewrites clear_expired to (a) call delete_internal instead of the public delete, and (b) guard the data lookup with if let Some(..) instead of .unwrap(). Public delete is refactored to clear_expired_if_auto(); delete_internal(key), which is behaviorally identical to before. Three regression tests are added.

Scope Alignment

Fully met. Every stated requirement is satisfied and I verified each independently:

• Recursion no longer possible via delete (see below).
• No panic on stale heap entries.
• Public delete behavior unchanged.
• Overwritten-key and return-count semantics preserved.
• Regression tests fail pre-fix, pass post-fix.

No scope creep — the diff touches only src/lib.rs, and only the two functions plus the new tests. Out-of-scope items (heap pruning on delete, Config/TTL/capacity changes, concurrency) are correctly left untouched.

Correctness Verification

Item 1 — recursion eliminated (genuinely). The only re-entrant cycle was clear_expired → delete → clear_expired_if_auto → clear_expired, peeking the same not-yet-popped live expired entry each time. After the fix, clear_expired calls only delete_internal (which calls nothing re-entrant) and self.expiries.pop(). The only callers of clear_expired_if_auto are delete and set_with_ttl; clear_expired no longer calls either. clear_expired is now fully non-re-entrant. Confirmed empirically: on reconstructed pre-fix code the Item 1 test aborts with stack overflow / SIGABRT; on the fix it passes.

Item 2 — stale-entry guard is correct, heap still drained. On the None path (data.get returns None for a key deleted before its expiry), the if let body is skipped but self.expiries.pop() — a sibling statement inside the if exp_item.is_expired() block — still executes, so the stale entry is drained and the count is not incremented. I checked the brace nesting explicitly; pop() is not inside the if let. Confirmed empirically: pre-fix the Item 2 test panics at Option::unwrap() on None; post-fix it passes and expiries.len() == 0.

Overwritten-key case preserved. When data.get returns Some but expired_at differs (key overwritten with a longer TTL, leaving a stale shorter-expiry heap entry), the inner equality check is false: not counted, not deleted, but still popped — identical to pre-fix. The existing test_clear_expired_with_overwritten_key continues to pass.

Public delete unchanged. delete_internal = self.data.remove(key).map(|item| item.value) is exactly equivalent to the old let item = self.data.remove(key)?; Some(item.value). Auto-clear still fires first; the heap entry is still left in place. test_delete confirms.

Test Coverage

Strong. The three new tests map 1:1 to the two bugs plus a combined scenario, and I verified each genuinely fails on pre-fix code (Item 1: stack overflow/abort; Item 2 and combined: unwrap-on-None panic). They assert the right post-conditions: data presence, heap drain (expiries.len()), and cleared_count. No weak/trivially-true assertions.

One small additive opportunity (nice-to-have, not required): there's no direct test asserting that the public delete still leaves the heap entry in place AND still fires auto-clear in the same case — test_delete covers the heap-leave behavior, and the new tests cover the no-recursion path, so the behavior is transitively covered. Fine as-is.

Security Issues

None. No new inputs, no unsafe, no external surface. The change strictly removes a panic and an unbounded-recursion DoS — a security improvement (a single expired entry under default config previously crashed the process).

Code Quality

Clean and minimal. The delete_internal extraction is the right factoring, and the doc comment accurately describes the no-auto-clear / no-heap-prune contract. Two trivial, non-blocking notes:

• The "Does not delete from BinaryHeap" comment moved from delete to delete_internal. Accurate, but a reader looking only at public delete no longer sees it. Negligible.
• No version bump in Cargo.toml (still 0.2.0). Appropriate for a fix PR where you control release timing; flag it only so you remember a patch bump before the next publish.

Verification Run (on the PR branch)

• cargo test — 27 passed, 0 failed.
• cargo clippy --all-targets -- -D warnings — clean.
• cargo fmt --check — clean.
• Commit has no Claude co-authorship trailer (matches repo convention).

Summary and Recommended Actions

• Overall verdict: Ready to merge.
• Blockers: none.
• Non-blockers: none.
• Nice-to-haves: consider a patch version bump before the next publish; optionally a direct assertion that public delete leaves the heap entry while firing auto-clear.

Both reported defects are genuinely fixed, the fix is minimal and correct, edge cases (stale entry, overwritten key, count semantics, public delete) are preserved, and the regression tests provably exercise the bug paths.

Recommended merge commit message:

fix: prevent clear_expired infinite recursion and stale-entry panic

clear_expired previously recursed unboundedly via delete -> clear_expired_if_auto
on a live expired entry under default config, and panicked on data.get().unwrap()
for a heap entry whose key was deleted before expiry. Add a private delete_internal
that skips the auto-clear hook, call it from both delete and clear_expired, and
guard the data lookup with if-let. Public delete behavior is unchanged. Adds three
regression tests covering the recursion path, the stale-entry path, and both combined.