Fix invalid timestamp comparison order for updates on unevicted expired entries - #581
Merged
tatsuya6502 merged 3 commits intoMar 15, 2026
Merged
Conversation
Exposed by moka-rs#564, which sets a None expiration time when the expire_after_update callback returns None. The underlying problem - comparing non sequential timestamps, has existed for some time, but the buggy behavior was introduced by that PR. Reproduces a bug where `optionally_get_with` on an expired but not yet evicted entry causes the new value's expiration to be cleared, making it never expire. 1. optionally_get_with misses at time 0, inserts value 1 with 2s expr 2. Housekeeping will happen at 0.3, 0.6, 0.9, 1.2, 1.5, 1.8s 2. optionally_get_with hits (entry not expired yet) at time 1.98s. 3. optionally_get_with misses (expired) at 2.01s - Since housekeeping has not happened, the entry is not evicted, meaning that this is an UPDATE operation in do_insert_with_hash. - In do_insert_with_hash, the 'current' time (ts) is _after_ the entry expiration - In expire_after_read_or_update (line 694) called from do_post_update_steps, the expiration time is before the current time, resulting in a None value - None is passed as the current_time to the default expire_after_update, which returns None - duration is now None and current_per_entry_exp_time is Some, resulting in no expiration 4. optionally_get_with HITS value 2 at time 12.01s, and indefinitely after that
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughEnsure per-entry expiration never yields Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/sync/cache.rs (1)
5296-5297: Strengthen the final assertion to validate exact behavior.
assert_ne!(value, Some(2))is a bit loose for a regression test. AssertingSome(3)makes the expected re-initialization path explicit and guards against false positives.Suggested assertion tightening
- let value = cache.optionally_get_with("key", || next_value()); - assert_ne!(value, Some(2), "Access at 12.01s should not still be 2"); + let value = cache.optionally_get_with("key", || next_value()); + assert_eq!(value, Some(3), "Access at 12.01s should reinitialize to 3");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sync/cache.rs` around lines 5296 - 5297, Replace the loose inequality assertion with an exact equality check so the test verifies re-initialization: in the block using cache.optionally_get_with("key", || next_value()) replace the assert_ne!(value, Some(2), ...) with an assertion that value equals Some(3) (e.g. assert_eq!(value, Some(3), "...")) so the test explicitly validates the expected re-initialized value returned by next_value().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/sync/cache.rs`:
- Around line 5227-5238: Update the test narrative comment to correct numbering,
fix the typo "expr" to "expiry", and replace the incorrect phrase "None is
passed as the current_time" with "None is passed as current_duration"; keep
references to the relevant functions/locations (optionally_get_with,
housekeeping, do_insert_with_hash, do_post_update_steps,
expire_after_read_or_update, and default expire_after_update) so the sequence
reads clearly and accurately that an UPDATE occurs because housekeeping hasn't
evicted the expired entry and the expiration logic receives no current_duration,
causing the duration to be None while current_per_entry_exp_time is Some.
---
Nitpick comments:
In `@src/sync/cache.rs`:
- Around line 5296-5297: Replace the loose inequality assertion with an exact
equality check so the test verifies re-initialization: in the block using
cache.optionally_get_with("key", || next_value()) replace the assert_ne!(value,
Some(2), ...) with an assertion that value equals Some(3) (e.g.
assert_eq!(value, Some(3), "...")) so the test explicitly validates the expected
re-initialized value returned by next_value().
Comment on lines
+5227
to
+5238
| /// 1. optionally_get_with misses at time 0, inserts value 1 with 2s expr | ||
| /// 2. Housekeeping will happen at 0.3, 0.6, 0.9, 1.2, 1.5, 1.8s | ||
| /// 2. optionally_get_with hits (entry not expired yet) at time 1.98s. | ||
| /// 3. optionally_get_with misses (expired) at 2.01s | ||
| /// - Since housekeeping has not happened, the entry is not evicted, meaning that this is | ||
| /// an UPDATE operation in do_insert_with_hash. | ||
| /// - In do_insert_with_hash, the 'current' time (ts) is _after_ the entry expiration | ||
| /// - In expire_after_read_or_update (line 694) called from do_post_update_steps, the | ||
| /// expiration time is before the current time, resulting in a None value | ||
| /// - None is passed as the current_time to the default expire_after_update, which returns | ||
| /// None | ||
| /// - duration is now None and current_per_entry_exp_time is Some, resulting in no expiration |
There was a problem hiding this comment.
Fix inaccuracies in the test narrative comment.
The comment has minor correctness/readability issues: duplicated step numbers, typo (expr), and it says None is passed as current_time (should describe current_duration in this context).
Suggested doc comment cleanup
- /// 1. optionally_get_with misses at time 0, inserts value 1 with 2s expr
- /// 2. Housekeeping will happen at 0.3, 0.6, 0.9, 1.2, 1.5, 1.8s
- /// 2. optionally_get_with hits (entry not expired yet) at time 1.98s.
+ /// 1. optionally_get_with misses at time 0, inserts value 1 with 2s expiry
+ /// 2. Housekeeping will happen at 0.3, 0.6, 0.9, 1.2, 1.5, 1.8s
+ /// 3. optionally_get_with hits (entry not expired yet) at time 1.98s.
- /// 3. optionally_get_with misses (expired) at 2.01s
+ /// 4. optionally_get_with misses (expired) at 2.01s
...
- /// - None is passed as the current_time to the default expire_after_update, which returns
+ /// - None is passed as the current_duration to the default expire_after_update, which returns🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/sync/cache.rs` around lines 5227 - 5238, Update the test narrative
comment to correct numbering, fix the typo "expr" to "expiry", and replace the
incorrect phrase "None is passed as the current_time" with "None is passed as
current_duration"; keep references to the relevant functions/locations
(optionally_get_with, housekeeping, do_insert_with_hash, do_post_update_steps,
expire_after_read_or_update, and default expire_after_update) so the sequence
reads clearly and accurately that an UPDATE occurs because housekeeping hasn't
evicted the expired entry and the expiration logic receives no current_duration,
causing the duration to be None while current_per_entry_exp_time is Some.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## chore/add-tests-from-pr581 #581 +/- ##
=============================================================
Coverage ? 93.43%
=============================================================
Files ? 44
Lines ? 16960
Branches ? 0
=============================================================
Hits ? 15847
Misses ? 1113
Partials ? 0 🚀 New features to boost your workflow:
|
tatsuya6502
self-requested a review
March 2, 2026 10:29
Contributor
|
I think there is still one issue left (PR #582 contains both fixes). If this PR is merged, I could rebase accordingly. |
tatsuya6502
approved these changes
Mar 15, 2026
tatsuya6502
left a comment
tatsuya6502
left a comment
Member
There was a problem hiding this comment.
Thank you for the contribution. I took the fix from #582, but I'll try to take the test from this PR.
Member
|
Merging into a branch |
tatsuya6502
merged commit Mar 15, 2026
24b917e
into
moka-rs:chore/add-tests-from-pr581
25 checks passed
tatsuya6502
added a commit
that referenced
this pull request
Mar 15, 2026
Take the test case from #581
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
3 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.
Demonstrates and potentially fixes issue #575
When calling optionally_get_with on a key that is expired but not yet evicted by housekeeping, the subsequent 'update' operation compares two out-of-order timestamps, resulting in an inadvertent 'None' value. This causes the cache to keep the affected entry forever.
Exposed by PR #564, published in 0.12.13, which sets a None expiration time when the expire_after_update callback returns None. The underlying problem - comparing non sequential timestamps, has existed for some time, but the buggy behavior was introduced by that PR.
Test case reproduces a bug where
optionally_get_withon an expired but not yet evicted entry causes the new value's expiration to be cleared, making it never expire.Summary by CodeRabbit
Bug Fixes
Tests