-
-
Notifications
You must be signed in to change notification settings - Fork 116
Description
Originally posted by @coderabbitai[bot] in #564 (review)
2907-2997: Ensure this test always exercises theexpire_after_updatepath, not justexpire_after_create.The scenario and assertions look correct for the bug you’re targeting (clearing per-entry expiration when
expire_after_updatereturnsNone), but there’s a subtle dependency on current eviction/timer behavior:
- With
Duration::ZEROandcache.run_pending_tasks()right after the first insert, the timer wheel may fully evict the entry before the secondinsert, causing that second call to go through the insert path (expire_after_create) instead of the update path (expire_after_update).- If that happens, the test would still pass even if the sync fix in
expire_after_read_or_updateregressed, because the problematicexpire_after_updatecode path would not be covered.It would be safer to make the “expired but still present” state explicit and independent of current timer-wheel internals, for example by:
- Using
Clock::mock()with a small non-zero per-entry duration, and:
- Insert +
run_pending_tasks()to admit the entry.- Advance the mock clock past the per-entry TTL without calling
run_pending_tasksso the entry remains in the map butget()treats it as expired.- Flip
should_expiretofalseand then callinsertagain, ensuring it’s an in-place update that triggersexpire_after_update.- Or, more minimally, avoid the initial
run_pending_tasks()so that the first entry is guaranteed to still be present in the CHT when the secondinserthappens, relying only onget()’s expiration checks for the first “expired” assertion.This isn’t a blocker, but I’d recommend double-checking (or instrumenting once) that the second
insertcurrently hitsexpire_after_update, and consider tightening the test so that it continues to do so even if eviction/timer internals change later.