Take the test case from #581 - #583
Merged
Merged
Conversation
Exposed by #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
Fix invalid timestamp comparison order for updates on unevicted expired entries
📝 WalkthroughWalkthroughAdds a unit test Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #583 +/- ##
==========================================
+ Coverage 93.33% 93.45% +0.11%
==========================================
Files 44 44
Lines 17036 17073 +37
==========================================
+ Hits 15901 15955 +54
+ Misses 1135 1118 -17 🚀 New features to boost your workflow:
|
tatsuya6502
marked this pull request as ready for review
March 15, 2026 11:12
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/sync/cache.rs (2)
5404-5405: Assert the exact expected value at Line 5405.
assert_ne!(..., Some(2))is too permissive and can hide unrelated regressions. This scenario should deterministically returnSome(3).♻️ Proposed fix
- 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 miss expired value 2 and create value 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 5404 - 5405, The assertion at the optionally_get_with call is too permissive (assert_ne!(..., Some(2))) and should assert the exact expected value; change the assertion for the result of cache.optionally_get_with("key", next_value) to assert it equals Some(3) so the test deterministically verifies the expected value returned by that call.
5335-5347: Simplify the long scenario comment to avoid stale internals.The block currently encodes internal function-path assumptions and a fixed line reference, which will age quickly. Keep the test docs behavior-focused.
🧹 Suggested comment rewrite
- /// 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 + /// 1. `optionally_get_with` misses at t=0 and inserts value 1 with 2s expiry. + /// 2. At t=1.98s, `optionally_get_with` still hits value 1. + /// 3. At t=2.01s (expired, not yet evicted), `optionally_get_with` should miss and create value 2. + /// 4. At t=12.01s, value 2 should also be expired; `optionally_get_with` should create a new value.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sync/cache.rs` around lines 5335 - 5347, The long scenario comment for the test should be simplified to a behavior-focused description: remove internal function-path assumptions, fixed line references, and implementation details (e.g., do_insert_with_hash, do_post_update_steps, expire_after_read_or_update, expire_after_update, current_per_entry_exp_time) and instead state the observable sequence of events and expected outcomes for optionally_get_with (miss, insert with 2s expiry, intermediate housekeeping timings, hit before expiry, miss after expiry, and final behavior), keeping only what a reader needs to understand the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/sync/cache.rs`:
- Around line 5404-5405: The assertion at the optionally_get_with call is too
permissive (assert_ne!(..., Some(2))) and should assert the exact expected
value; change the assertion for the result of cache.optionally_get_with("key",
next_value) to assert it equals Some(3) so the test deterministically verifies
the expected value returned by that call.
- Around line 5335-5347: The long scenario comment for the test should be
simplified to a behavior-focused description: remove internal function-path
assumptions, fixed line references, and implementation details (e.g.,
do_insert_with_hash, do_post_update_steps, expire_after_read_or_update,
expire_after_update, current_per_entry_exp_time) and instead state the
observable sequence of events and expected outcomes for optionally_get_with
(miss, insert with 2s expiry, intermediate housekeeping timings, hit before
expiry, miss after expiry, and final behavior), keeping only what a reader needs
to understand the test.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/sync/cache.rs (2)
5395-5396: Consider a more specific assertion to verify expected behavior.The current assertion
assert_ne!(value, Some(2))correctly detects if the bug exists (stale value 2), butassert_eq!(value, Some(3))would additionally verify that a new value was properly created with fresh expiry. This makes test failures more descriptive.💡 Suggested improvement
let value = cache.optionally_get_with("key", next_value); - assert_ne!(value, Some(2), "Access at 12.01s should not still be 2"); + assert_eq!(value, Some(3), "Access at 12.01s should create value 3 since value 2 expired");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sync/cache.rs` around lines 5395 - 5396, Replace the non-specific inequality check with an explicit equality check so the test verifies a fresh value was created: in the call to cache.optionally_get_with("key", next_value) assert that value == Some(3) instead of assert_ne!(value, Some(2)); update the assertion in the test around the optionally_get_with invocation (refer to the variables value and next_value and the optionally_get_with function) so failures show the expected new value rather than just the absence of the stale value.
5340-5341: Redundant import:Clockis already imported at module level.
Clockis already imported at line 1884 (use crate::common::{time::Clock, HousekeeperConfig}), so this local import is unnecessary.🧹 Suggested fix
#[test] fn test_optionally_get_with_expired_entry_bug() { - use crate::common::time::Clock; use std::sync::atomic::{AtomicU32, Ordering};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sync/cache.rs` around lines 5340 - 5341, In test_optionally_get_with_expired_entry_bug remove the redundant local import `use crate::common::time::Clock;` since `Clock` is already imported at module level (from `use crate::common::{time::Clock, HousekeeperConfig}`); simply delete that local use statement so the test compiles without duplicate imports and continues to reference `Clock` from the module-level import.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/sync/cache.rs`:
- Around line 5395-5396: Replace the non-specific inequality check with an
explicit equality check so the test verifies a fresh value was created: in the
call to cache.optionally_get_with("key", next_value) assert that value ==
Some(3) instead of assert_ne!(value, Some(2)); update the assertion in the test
around the optionally_get_with invocation (refer to the variables value and
next_value and the optionally_get_with function) so failures show the expected
new value rather than just the absence of the stale value.
- Around line 5340-5341: In test_optionally_get_with_expired_entry_bug remove
the redundant local import `use crate::common::time::Clock;` since `Clock` is
already imported at module level (from `use crate::common::{time::Clock,
HousekeeperConfig}`); simply delete that local use statement so the test
compiles without duplicate imports and continues to reference `Clock` from the
module-level import.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/sync/cache.rs (1)
5394-5395: Strengthen the final assertion to an exact expected value.At Line 5395,
assert_ne!(value, Some(2))is weaker than needed in a deterministic single-threaded flow. Prefer assertingSome(3)so the test guarantees exactly one new creation at the final step.🎯 Proposed assertion update
- 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 miss and return 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 5394 - 5395, Update the final assertion that checks the result of cache.optionally_get_with("key", next_value): replace the weak inequality check against Some(2) with an exact equality check for the expected value Some(3) so the test verifies the precise result of the single-threaded creation; locate the assertion that inspects the variable value after calling optionally_get_with and change it to assert equality with Some(3).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/sync/cache.rs`:
- Around line 5394-5395: Update the final assertion that checks the result of
cache.optionally_get_with("key", next_value): replace the weak inequality check
against Some(2) with an exact equality check for the expected value Some(3) so
the test verifies the precise result of the single-threaded creation; locate the
assertion that inspects the variable value after calling optionally_get_with and
change it to assert equality with Some(3).
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.
Summary by CodeRabbit