refactor: bound expiry index with a 1:1 BTreeMap (fixes unbounded heap growth)

[uzyn]

Summary

SparKV tracked expirations in a BinaryHeap<ExpEntry> with lazy deletion: delete and overwriting set left stale ExpEntry tombstones behind because a binary heap can't remove an arbitrary node cheaply. The index was therefore not bounded by the live key count — with auto_clear_expired = false (or an idle store) it grew without bound, a real memory leak.

This change replaces the heap with a BTreeMap keyed by (expired_at, key) maintained 1:1 with data: every set/delete updates the index so it can never hold a tombstone. That bounds memory to the live set exactly, and lets clear_expired drop its tombstone-reconciliation logic entirely (the same logic that produced bugs #2/#3). Public API behavior is unchanged; the only public-surface change is removing the now-dead ExpEntry type — a breaking change handled by a 0.2.0 -> 0.3.0 bump.

Requirements

• Expiry index is bounded to the live set: after any sequence of set/delete, expiries.len() == data.len(); overwriting one key with auto_clear_expired = false keeps expiries.len() == 1.
• delete removes the corresponding index entry (no tombstone left behind).
• Overwriting an existing key replaces its index entry rather than adding a second.
• clear_expired removes exactly the expired entries from both data and the index, returns the count removed, and stops at the first non-expired entry — caller-observable behavior identical to before.
• Public API behavior unchanged: get / get_item / set / set_with_ttl / delete / contains_key / len / is_empty keep their semantics, including expiry-awareness and capacity/overwrite rules.
• ExpEntry type and src/expentry.rs deleted, along with mod expentry; / pub use expentry::ExpEntry;.
• Version bumped 0.2.0 -> 0.3.0 in Cargo.toml and Cargo.lock.
• cargo test, cargo clippy -- -D warnings, and cargo fmt --check all pass.

Out of scope

• The rebuild-threshold alternative (keeping the heap) — explicitly not chosen.
• Any change to len / is_empty physical-vs-live semantics (post-fix: restore KvEntry Eq, make contains_key expiry-aware, fix clear_expired crashes #3 behavior kept as-is).
• KvEntry's PartialEq/Eq derive — left untouched; now only used by its own unit test.
• Edition upgrade (crate stays on 2021).
• Closing PR fix: prevent clear_expired infinite recursion and stale-entry panic #2 (superseded by fix: restore KvEntry Eq, make contains_key expiry-aware, fix clear_expired crashes #3) — separate housekeeping.
• Publishing to crates.io / releasing.

Technical approach

• Field: expiries: BTreeMap<(Instant, String), ()>. Keying by (expired_at, key) keeps entries ordered by expiry (earliest at the front) and disambiguates two keys sharing an Instant. Invariant: for every key K in data, the index holds exactly one entry (expired_at(K), K) and nothing else.
• set_with_ttl: insert into data; if it returned a previous entry (overwrite), remove that entry's old index key; then insert the new index key. Keeps the index 1:1.
• delete: remove from data, then remove (expired_at, key) from the index.
• clear_expired: front-pop loop using first_key_value / pop_first while the earliest entry's expired_at < now. With no tombstones, every popped index entry maps to a real data entry, so the old key/expired_at match check is gone. < now preserves the strict comparison the old ExpEntry::is_expired used. Removal goes through data.remove (not self.delete) to keep the re-entrant auto-clear guard from fix: restore KvEntry Eq, make contains_key expiry-aware, fix clear_expired crashes #3.
• Deleted src/expentry.rs and its mod/pub use lines; removed all BinaryHeap/ExpEntry usage.

Test plan

• New regression test test_expiry_index_stays_bounded_to_live_set (written first, confirmed FAILING on the old BinaryHeap code: expiries.len() was 1000, expected 1): with auto_clear_expired = false, overwrites one key 1000x and asserts expiries.len() == 1 and data.len() == 1; then deletes and asserts both are 0. Passes after the swap — executable proof of the leak fix.
• Rewrote three tests that encoded the old tombstone invariant: test_set_get (overwrite now keeps expiries.len() at 1, not 2), test_delete (index drops to 0, not 1), test_clear_expired_with_overwritten_key (overwrite updates the index 1:1, so 2 not 3). Updated their stale comments.
• All other tests pass unchanged. The four expentry.rs unit tests were removed with the file.

Results: all 25 tests pass, cargo clippy -- -D warnings clean, cargo fmt --check clean.

Notes

• Write path trades O(1) heap push for O(log n) BTreeMap insert/remove — negligible for this store and the win is a hard memory bound plus simpler clear_expired.
• Minor deviation from the plan: test_clear_expired_does_not_panic_on_deleted_key (flagged "pass unchanged") still passes, but its two comments referenced the now-deleted ExpEntry/heap. Updated only those comments (no assertion changes) so the code doesn't reference a type that no longer exists.
• Follow-up: PR fix: prevent clear_expired infinite recursion and stale-entry panic #2 is superseded by fix: restore KvEntry Eq, make contains_key expiry-aware, fix clear_expired crashes #3 and can be closed as housekeeping (out of scope here).

[uzyn]

Standalone Review — PR #4: bound expiry index with a 1:1 BTreeMap

Changes Overview

This PR replaces SparKV's expiry index — previously a BinaryHeap<ExpEntry> with lazy (tombstone) deletion — with a BTreeMap<(Instant, String), ()> maintained 1:1 with the data HashMap. Every set/delete now updates the index in lockstep, so it can never hold a stale tombstone, bounding index memory exactly to the live key count and fixing the unbounded-growth leak. The dead ExpEntry type and src/expentry.rs are removed, and the crate is bumped 0.2.0 -> 0.3.0 (breaking: ExpEntry no longer exported).

Scope Alignment

Fully met. Every requirement and technical-approach item in the PR description is delivered and independently verified:

• Index bounded to the live set; expiries.len() == data.len() after any set/delete sequence (proven by the new 1000x-overwrite regression test).
• delete removes the matching index entry (lib.rs:44), no tombstone left.
• Overwriting a key replaces its index entry 1:1 (lib.rs:137-140), not append.
• clear_expired removes exactly the expired entries from both structures and returns the count (lib.rs:73-87); caller-observable behavior unchanged.
• ExpEntry, src/expentry.rs, and the mod/pub use lines are gone; no stray references remain anywhere in src/ or README.md (grep confirms).
• Version bumped in both Cargo.toml and Cargo.lock.

No scope creep. The change is confined to the expiry-index mechanism plus the necessitated test/version updates.

Potential Bugs

None found. I verified the invariant on every mutation path:

• Overwrite path (lib.rs:137-140): index keys are built consistently as key.to_string() for both the removal of the old entry and the insertion of the new; the removed key (old.expired_at, key) exactly matches what the prior set inserted. Net index delta on overwrite is zero. Correct.
• Capacity/size/TTL-rejected set: all three guards (ensure_capacity_ignore_key, ensure_item_size, ensure_max_ttl) run and ?-return before any data.insert/expiries.insert (lib.rs:131-133), so a rejected set leaves both structures untouched and consistent. Correct.
• Delete path (lib.rs:43-46): removes (item.expired_at, item.key) using the stored entry's own fields — exactly the index key that was inserted. Disjoint partial move of key into the tuple then value out of the struct is valid (clean compile confirms). Correct.
• clear_expired boundary (lib.rs:79): uses strict *expired_at < now, identical to the old ExpEntry::is_expired (expired_at < now). No semantic drift. The front-pop loop re-checks first_key_value each iteration and pop_first removes the same front element, so peek/pop cannot desync.
• Equal Instants across keys: the (Instant, String) composite key keeps two same-Instant entries distinct via their differing String, so none is lost or double-counted. Correct.

Test Coverage

Strong. The three rewritten tests genuinely encode the NEW correct invariant rather than being weakened to pass:

• test_set_get — overwrite now asserts expiries.len() == 1 (was 2); directly proves 1:1 replacement.
• test_delete — asserts expiries.len() == 0 after delete (was 1); proves no tombstone.
• test_clear_expired_with_overwritten_key — asserts expiries.len() == 2 (was 3) after overwrite; proves the index, not just data, is updated on overwrite.

The new test_expiry_index_stays_bounded_to_live_set is the executable proof of the fix (1000 overwrites of one key, asserts expiries.len() == 1 and data.len() == 1, then deletes to 0). Per the PR notes this was confirmed FAILING on the old heap (len was 1000). The capacity-rejected-set consistency path is exercised indirectly by test_set_should_fail_if_capacity_exceeded, and equal-Instant handling by test_clear_expired (multiple short-TTL entries). Coverage is adequate; I see no untested reachable path.

I ran the full suite myself in the worktree: 25 passed, 0 failed.

Security Issues

None. No new dependencies, no untrusted input, no unsafe, no secrets. The composite-key construction is allocation-only and panic-free.

Code Quality

Clean. clear_expired's first_key_value().is_some_and(...) + pop_first() reads clearly and removes the entire tombstone-reconciliation block (and its key/expired_at match check) that previously caused bugs #2/#3. Comments are minimal and accurate; stale comments referencing the deleted ExpEntry/heap were updated, not left dangling. Files end with a single trailing newline. cargo clippy -- -D warnings and cargo fmt --check both pass clean.

Project Conventions

• No Claude co-authorship in the commit (verified full message).
• Edition stays 2021 as scoped.
• No stray .PLAN.md in the diff or worktree.
• Single trailing blank line on all changed files.
• Minimal comments, consistent with project style.

Summary and Recommended Actions

• Overall verdict: Ready to merge.
• Blockers: none.
• Non-blockers: none.
• Nice-to-haves: The README has a "Migrating from 0.1 to 0.2" section but no 0.2 -> 0.3 note, even though removing ExpEntry from the public surface is a breaking change. A one-line migration note (ExpEntry removed; it was internal-only) would help downstream users. Out of this PR's stated scope and purely documentation — track separately.

Verified independently: 25/25 tests pass, clippy clean, fmt clean, 1:1 invariant holds on overwrite/delete/rejected-set/clear_expired, boundary semantics match the old code, equal Instants disambiguated correctly, no stale ExpEntry/BinaryHeap references.

refactor: bound expiry index with a 1:1 BTreeMap (fixes unbounded heap growth)

Replace the lazily-deleted BinaryHeap<ExpEntry> expiry index with a
BTreeMap<(Instant, String), ()> maintained 1:1 with `data`, so the index
can never hold a tombstone and memory stays bounded to the live set. Drop
the tombstone-reconciliation logic in clear_expired. Remove the now-dead
ExpEntry type and src/expentry.rs. Breaking: ExpEntry no longer exported;
bump 0.2.0 -> 0.3.0.