feat: implement TTL#139
Merged
aviralgarg05 merged 6 commits intoFeb 22, 2026
Merged
Conversation
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughAdds TTL (time-to-live) support to SemanticCache: entries gain timestamps; expired entries are detected and evicted on get, explain, load, and via a new public set_cache_expiration API; cache persistence format bumped to 1.1 to include max_age_seconds; tests for TTL behavior added. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SemanticCache
participant Disk as "Cache JSON"
participant Time
Client->>SemanticCache: put(query, result)
SemanticCache->>Time: now()
SemanticCache-->>SemanticCache: store entry with timestamp
Client->>SemanticCache: get(query)
SemanticCache->>Time: now()
SemanticCache-->>SemanticCache: _is_entry_expired? -> if expired _evict_expired()
alt entry valid
SemanticCache-->>Client: return result
else expired or missing
SemanticCache-->>Client: return None
end
Client->>SemanticCache: save_cache_json(filepath)
SemanticCache->>Disk: write entries + max_age_seconds + format_version 1.1
Client->>SemanticCache: load_cache_json(filepath)
SemanticCache->>Disk: read JSON
SemanticCache->>SemanticCache: restore max_age_seconds
SemanticCache->>Time: now()
SemanticCache-->>SemanticCache: _evict_expired()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🎉 Thanks for opening your first pull request in NexumDB!
We appreciate your contribution! Here's what happens next:
- ✅ Automated checks will run (CI, tests, linting)
- 👀 A maintainer will review your changes
- 💬 We may suggest some improvements or ask questions
- 🚀 Once approved, we'll merge your contribution!
Before we can merge:
- Make sure all CI checks pass
- Sign your commits with DCO (
git commit -s) - Address any review feedback
Check out our Contributing Guide for more details.
Thanks for making NexumDB better! 🙌
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
nexum_ai/optimizer.py (2)
153-169: 🧹 Nitpick | 🔵 TrivialExpired entries accumulate in memory —
get()skips but never removes them.
get()skips expired entries but does not evict them, so they remain inself.cacheindefinitely untilset_cache_expirationor_evict_expiredis called explicitly. For long-running processes with heavy cache usage, this could cause gradual memory growth.Consider either (a) running
_evict_expired()periodically (e.g., every N calls or on a time interval), or (b) noting this as intended behavior in the docstring so future maintainers are aware.
438-445:⚠️ Potential issue | 🟡 Minor
cache_entries_checkedincludes expired entries — field name is misleading.Line 440 reports
len(self.cache)ascache_entries_checked, but expired entries are skipped at lines 404–405 and never actually "checked" for similarity. This makes the field name inaccurate. The test on line 221 even acknowledges it with a comment# total in list.Consider either renaming to
total_cache_entries(accurate) or computing the actual number of non-expired entries that were checked.♻️ Proposed fix — track actual entries checked
+ entries_checked = 0 # Analyze cache entries safely (skip expired) for i, entry in enumerate(self.cache): if self._is_entry_expired(entry): continue + entries_checked += 1 try: similarity = self.cosine_similarity(query_vec, entry.get('vector', [])) ... return { 'query': query, - 'cache_entries_checked': len(self.cache), + 'cache_entries_checked': entries_checked,
🤖 Fix all issues with AI agents
In `@nexum_ai/optimizer.py`:
- Around line 338-352: get_cache_stats currently does a check-then-act on
self.cache_path (uses exists() then stat()) which can raise an
OSError/FileNotFoundError if the file is removed concurrently; replace that
TOCTOU pattern with a safe stat wrapped in try/except. Add a helper like
_safe_cache_size_bytes (or inline) that calls self.cache_path.stat() inside
try/except OSError and returns 0 on failure, then use that helper in
get_cache_stats instead of the exists()/stat() pair; keep references to
get_cache_stats, self.cache_path.stat(), self.cache_path.exists(), and any
clear() caller that might remove the file.
- Around line 324-328: load_cache_json restores max_age_seconds but doesn't
evict entries that expired during downtime, leaving stale items counted in
get_cache_stats()['total_entries']; after assigning self.max_age_seconds in
load_cache_json call self._evict_expired() to remove stale entries (so get()
behavior and stats align), ensuring you reference the load_cache_json method,
the max_age_seconds attribute, and the _evict_expired() helper when making the
change.
- Around line 120-136: Currently _is_entry_expired calls time.time() per entry
which causes redundant syscalls and inconsistent TTL checks; change its
signature to accept an optional now parameter (e.g., def _is_entry_expired(self,
entry: Dict, now: Optional[float]=None) -> bool) and use time.time() only when
now is None, then update callers (_evict_expired, get, explain_query and any
other loops) to snapshot now = time.time() once before iterating and pass that
now into _is_entry_expired to perform consistent, single-snapshot expiration
checks.
In `@nexum_ai/tests/test_optimizer.py`:
- Around line 211-222: The test test_explain_query_skips_expired is asserting
cache_entries_checked == 1 which ties the test to the current (ambiguous)
implementation that counts total cached items including expired ones; update the
test to either (a) explicitly document the coupling by adding a clear
comment/TODO next to the cache_entries_checked assertion referencing
cache_entries_checked and optimizer.py's current semantics so future changes
don't silently break, or (b) if you intend cache_entries_checked to mean
"actually checked" update the assertion to 0 (and adjust any other expectations
accordingly). Refer to the test name test_explain_query_skips_expired and the
field cache_entries_checked when making the change so reviewers can find and
evaluate the intended stat semantics.
- Around line 5-6: Remove the unused symbol "patch" from the import statement in
the test file: delete "from unittest.mock import patch" (or remove "patch" if
combined) so only used imports remain (e.g., keep "import time"); ensure the
top-level imports in the test file no longer include "patch" and run
linters/tests to confirm no unused-import warnings.
- Around line 133-140: The test currently imports pytest inside
test_set_cache_expiration_rejects_non_positive; move the inline "import pytest"
to the module-level imports at the top of the file and remove the in-function
import; update the top imports section so pytest is imported alongside other
test imports and leave the test_set_cache_expiration_rejects_non_positive
function (which uses SemanticCache and calls cache.set_cache_expiration)
unchanged otherwise.
aviralgarg05
requested changes
Feb 16, 2026
aviralgarg05
left a comment
aviralgarg05
left a comment
Owner
There was a problem hiding this comment.
Resolve the issues and failing workflows
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nexum_ai/optimizer.py (1)
456-458:⚠️ Potential issue | 🟡 Minor
cache_entries_checkedincludes expired (skipped) entries, making the name misleading.Line 458 reports
len(self.cache)which counts all entries including expired ones that were skipped at lines 422-423. The field namecache_entries_checkedimplies only entries that were actually analyzed.Consider either renaming to
total_cache_entriesor computing the actual count of non-expired entries that were checked.♻️ Option A: Rename field
- 'cache_entries_checked': len(self.cache), + 'total_cache_entries': len(self.cache),♻️ Option B: Report actually-checked count
return { 'query': query, - 'cache_entries_checked': len(self.cache), + 'cache_entries_checked': len(cache_analysis),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nexum_ai/optimizer.py` around lines 456 - 458, The return dict currently sets 'cache_entries_checked' to len(self.cache) which includes expired/skipped entries; either rename the key to 'total_cache_entries' or compute the number of non-expired entries actually examined (the same logic used when skipping expired entries around the expired-check at lines that reference self.cache skipping) and set that count under 'cache_entries_checked'; update any callers or tests accordingly and keep the key consistent with the chosen behavior in the function that iterates self.cache.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nexum_ai/optimizer.py`:
- Around line 466-494: The method set_cache_expiration currently only accepts
positive numbers and offers no way to disable TTL; change
set_cache_expiration(self, max_age_hours: Optional[float] = 24) to accept None
(or add a new public disable_cache_expiration(self) that sets TTL off), update
the docstring and type hints to allow None, remove the ValueError for None (keep
it for non-positive numeric values), set self.max_age_seconds = None when
max_age_hours is None (and skip calling _evict_expired in that case), and adjust
the logger message to indicate TTL was disabled when None is passed; ensure
callers and any save_cache/save_cache_json persistence logic handle None
correctly.
---
Outside diff comments:
In `@nexum_ai/optimizer.py`:
- Around line 456-458: The return dict currently sets 'cache_entries_checked' to
len(self.cache) which includes expired/skipped entries; either rename the key to
'total_cache_entries' or compute the number of non-expired entries actually
examined (the same logic used when skipping expired entries around the
expired-check at lines that reference self.cache skipping) and set that count
under 'cache_entries_checked'; update any callers or tests accordingly and keep
the key consistent with the chosen behavior in the function that iterates
self.cache.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nexum_ai/optimizer.py (1)
456-458:⚠️ Potential issue | 🟡 Minor
cache_entries_checkedreports total entries, not entries actually checked.Line 458 returns
len(self.cache)which includes expired entries that were skipped. This is semantically misleading — consumers may assume this reflects the number of entries actually evaluated. Consider renaming tototal_cache_entriesor computing the actual count of non-expired entries checked.♻️ Proposed fix — report actual checked count
return { 'query': query, - 'cache_entries_checked': len(self.cache), + 'cache_entries_checked': len(cache_analysis), 'similarity_threshold': round(self.similarity_threshold, 4),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nexum_ai/optimizer.py` around lines 456 - 458, The return value currently reports 'cache_entries_checked' as len(self.cache), which includes expired/skipped entries; update the code in the method that builds this return dict so the metric correctly reflects the number of entries actually evaluated (or rename the field). Either compute the actual checked count by iterating over self.cache and counting non-expired entries that were examined (use the same expiration check logic used when skipping entries) and set 'cache_entries_checked' to that count, or change the key to 'total_cache_entries' and keep len(self.cache'; reference the existing 'cache_entries_checked' key and the 'self.cache' collection to locate the code to modify.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nexum_ai/optimizer.py`:
- Around line 332-341: In load_cache_json, restore the saved TTL state even when
saved_max_age is None so you don't keep a prior instance's TTL and incorrectly
evict entries; update the logic around saved_max_age (used to set
self.max_age_seconds) to always assign self.max_age_seconds =
float(saved_max_age) if saved_max_age is not None else None (preserving the
None/no-TTL state), then proceed to logger.info and call self._evict_expired()
as before; reference: load_cache_json, saved_max_age, self.max_age_seconds, and
_evict_expired.
In `@nexum_ai/tests/test_optimizer.py`:
- Around line 237-251: The load/save TTL behavior is missing the case where a
cache saved without TTL should clear any existing TTL on the loader; update
SemanticCache.load_cache_json to detect when the saved JSON does not include a
TTL field and explicitly set self.max_age_seconds = None (instead of leaving a
prior TTL from set_cache_expiration), and ensure entries are not evicted on load
simply because the loader previously had a TTL; reference SemanticCache,
load_cache_json, save_cache_json, set_cache_expiration, and max_age_seconds when
making the change.
---
Outside diff comments:
In `@nexum_ai/optimizer.py`:
- Around line 456-458: The return value currently reports
'cache_entries_checked' as len(self.cache), which includes expired/skipped
entries; update the code in the method that builds this return dict so the
metric correctly reflects the number of entries actually evaluated (or rename
the field). Either compute the actual checked count by iterating over self.cache
and counting non-expired entries that were examined (use the same expiration
check logic used when skipping entries) and set 'cache_entries_checked' to that
count, or change the key to 'total_cache_entries' and keep len(self.cache';
reference the existing 'cache_entries_checked' key and the 'self.cache'
collection to locate the code to modify.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
nexum_ai/optimizer.py (2)
160-177: 🧹 Nitpick | 🔵 TrivialLazy eviction in
get()means expired entries accumulate silently.
get()skips expired entries but never removes them. In a long-running process that never callsset_cache_expiration()again or reloads, expired entries pile up and are iterated every lookup, degrading performance over time. Consider opportunistically evicting (e.g., calling_evict_expired()periodically or after a threshold of skipped entries is hit).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nexum_ai/optimizer.py` around lines 160 - 177, The get() method currently skips expired entries via _is_entry_expired but never removes them, causing accumulation; modify get() (which uses vectorize(), cosine_similarity(), self.cache and self.similarity_threshold) to opportunistically evict expired entries—either call an existing _evict_expired() immediately when you detect an expired entry or track a small skipped_count and call _evict_expired() once the count exceeds a threshold (or after every N lookups) so expired items are removed and future lookups don’t degrade.
457-464: 🧹 Nitpick | 🔵 Trivial
cache_entries_checkedincludes expired entries that were skipped.
cache_entries_checkedis set tolen(self.cache)(line 459), but expired entries are skipped in the loop above (lines 423-424). The field name implies entries actually analyzed rather than total entries. This could confuse consumers of the API.Consider renaming or adding a separate field:
♻️ Suggested approach
+ entries_actually_checked = len(cache_analysis) + return { 'query': query, - 'cache_entries_checked': len(self.cache), + 'cache_entries_checked': entries_actually_checked, + 'total_cache_entries': len(self.cache), 'similarity_threshold': round(self.similarity_threshold, 4),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nexum_ai/optimizer.py` around lines 457 - 464, The field cache_entries_checked currently uses len(self.cache) but earlier logic skips expired entries; update the return to report the actual number of entries analyzed by either (a) using len(cache_analysis) for cache_entries_checked (since cache_analysis is the list of non-expired matches built in the loop) or (b) incrementing a checked_counter inside the matching loop and returning that value; if you still need the total stored entries, add a separate field like total_cache_entries = len(self.cache). Make this change in the function that builds the result (the return dict containing 'cache_entries_checked', 'best_match', 'top_matches', using variables cache_analysis and self.cache).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nexum_ai/optimizer.py`:
- Around line 467-505: In set_cache_expiration, validate that max_age_hours is
either None or a numeric type before comparing to 0 to avoid TypeError for
non-numeric inputs; e.g., check isinstance(max_age_hours, (int, float)) or use
numbers.Real and raise ValueError("max_age_hours must be a positive number or
None") if the check fails, then proceed with the existing None handling,
positive-value check, assignment to self.max_age_seconds, eviction via
self._evict_expired(), logging, and return.
- Around line 490-498: The set_cache_expiration path allows non-finite floats
(inf, nan) to slip through; update set_cache_expiration to validate
max_age_hours using math.isfinite and a positive check: if max_age_hours is None
keep the existing behavior, but if not, raise ValueError when not
math.isfinite(max_age_hours) or max_age_hours <= 0, and only then set
self.max_age_seconds = max_age_hours * 3600.0; reference the
set_cache_expiration function and the max_age_hours / max_age_seconds variables
when making the change.
In `@nexum_ai/tests/test_optimizer.py`:
- Around line 239-240: Move the inline imports of the standard-library modules
tempfile and os out of the test functions and hoist them to the module
top-level: remove the "import tempfile" and "import os" lines inside the test
methods (the ones around lines 239-240 and 255-256) and add a single pair of
imports at the top of nexum_ai/tests/test_optimizer.py; update any references in
the test functions so they use the top-level names without local imports.
- Around line 134-140: Add a test that ensures
SemanticCache.set_cache_expiration rejects NaN and Inf by creating a new test
method (e.g., test_set_cache_expiration_rejects_nan_and_inf) which constructs a
SemanticCache and asserts that calling set_cache_expiration(float('nan')) and
set_cache_expiration(float('inf')) raises an exception (ValueError or
TypeError); this will catch the current silent pass-through of NaN/Inf and guide
you to add proper isnan/isfinite validation inside set_cache_expiration if the
implementation needs to be hardened.
---
Outside diff comments:
In `@nexum_ai/optimizer.py`:
- Around line 160-177: The get() method currently skips expired entries via
_is_entry_expired but never removes them, causing accumulation; modify get()
(which uses vectorize(), cosine_similarity(), self.cache and
self.similarity_threshold) to opportunistically evict expired entries—either
call an existing _evict_expired() immediately when you detect an expired entry
or track a small skipped_count and call _evict_expired() once the count exceeds
a threshold (or after every N lookups) so expired items are removed and future
lookups don’t degrade.
- Around line 457-464: The field cache_entries_checked currently uses
len(self.cache) but earlier logic skips expired entries; update the return to
report the actual number of entries analyzed by either (a) using
len(cache_analysis) for cache_entries_checked (since cache_analysis is the list
of non-expired matches built in the loop) or (b) incrementing a checked_counter
inside the matching loop and returning that value; if you still need the total
stored entries, add a separate field like total_cache_entries = len(self.cache).
Make this change in the function that builds the result (the return dict
containing 'cache_entries_checked', 'best_match', 'top_matches', using variables
cache_analysis and self.cache).
Contributor
Author
|
All issues were fixed. Can you review this, please |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🚀 OSCG'26 Contributor Notice
NexumDB participates in the Open Source Contributor Games 2026! High-quality PRs earn you points, recognition, and networking opportunities. Please follow our contribution guidelines for maximum impact and ensure your submission meets OSCG quality standards.
Summary
Implement time-to-live (TTL) eviction for cache entries in
SemanticCache. Theset_cache_expirationmethod was previously a placeholder that only logged a message. This PR makes it fully functional — entries are now timestamped on creation, expired entries are transparently skipped during lookups, and the TTL setting persists across save/load cycles.Closes #117
What Changed?
nexum_ai/optimizer.pyimport timefor timestamp support.self.max_age_secondsattribute to__init__(defaults toNone— no TTL).put()now stores a'timestamp': time.time()in every cache entry._is_entry_expired(entry)checks if a single entry exceeds the TTL. Legacy entries without a timestamp are never considered expired (backward-compatible)._evict_expired()removes all expired entries and returns the count removed.get()transparently skips expired entries during similarity lookup.explain_query()skips expired entries during analysis.set_cache_expiration(max_age_hours)is fully implemented: sets the TTL, immediately evicts stale entries, returns eviction count, and raisesValueErroron non-positive input.get_cache_stats()includesmax_age_hoursandexpired_entrieswhen TTL is active.save_cache_json()persistsmax_age_seconds(format version bumped to1.1).load_cache_json()restoresmax_age_secondsso the TTL survives restarts.nexum_ai/tests/test_optimizer.pyimport timeandfrom unittest.mock import patch.TestSemanticCacheTTLclass with 12 tests covering:put()max_age_seconds)get()skipping expired entriesget()returning valid (non-expired) entriesNoneexplain_query()filtering expired entriesget_cache_stats()TTL fields_evict_expired()return count accuracyWhy?
The
set_cache_expirationmethod was listed as a placeholder (# For now, just a placeholder for TTL functionality). Without TTL, stale cache entries accumulate indefinitely, potentially returning outdated results. This implementation gives users control over cache freshness while maintaining full backward compatibility with existing caches.Type of Change
Testing
Local Testing Checklist
cargo fmt --all -- --check- Code formattingcargo clippy --workspace --all-targets -- -D warnings- Lintingcargo test --workspace -- --test-threads=1- All tests passcargo audit- No security vulnerabilitiespython -m ruff check nexum_ai/- Python linting (if applicable)python -m compileall nexum_ai- Python compilation (if applicable)Testing Details
python -m pytest nexum_ai/tests/test_optimizer.py -v -o "addopts="— all 32 tests passed (20 existing + 12 new TTL tests).get()skipping expired entries,explain_query()filtering, stats reporting, save/load persistence, legacy entry backward compatibility, input validation, and eviction count accuracy.Screenshots/Examples
Performance Impact
Security Considerations
Documentation
demo.sh(if applicable)Pre-Merge Checklist
main🎯 OSCG'26 Quality Standards
By submitting this PR, I confirm:
Additional Notes
1.0to1.1to reflect the newmax_age_secondsfield. Older versions will simply ignore the new field.nexum_ai/optimizer.pyandnexum_ai/tests/test_optimizer.py.Reviewers: Please check that all checkboxes are ticked before approving
Summary by CodeRabbit
New Features
Tests