Skip to content

Commit 2ff990e

Browse files
authored
Avoid a spurious assert in tests (#21543)
This avoids the following crash: ``` thread '<unnamed>' panicked at crates/sui-core/src/execution_cache/cache_types.rs:307:17: entry is newer than value 0x86aacc42979583a6797f4bb0cab20d436a1f41e215b2c87cb6e99ffdda5ec7db stack backtrace: 0: rust_begin_unwind at /rustc/4d91de4e48198da2e33413efdcd9cd2cc0c46688/library/std/src/panicking.rs:692:5 1: core::panicking::panic_fmt at /rustc/4d91de4e48198da2e33413efdcd9cd2cc0c46688/library/core/src/panicking.rs:75:14 2: sui_core::execution_cache::cache_types::MonotonicCache<K,V>::insert at /Users/marklogan/dev/sui-wb/crates/sui-core/src/execution_cache/cache_types.rs:307:17 3: sui_core::execution_cache::writeback_cache::WritebackCache::cache_latest_object_by_id at /Users/marklogan/dev/sui-wb/crates/sui-core/src/execution_cache/writeback_cache.rs:1210:12 4: sui_core::execution_cache::writeback_cache::WritebackCache::cache_object_not_found at /Users/marklogan/dev/sui-wb/crates/sui-core/src/execution_cache/writeback_cache.rs:1224:9 5: <sui_core::execution_cache::writeback_cache::WritebackCache as sui_core::execution_cache::ObjectCacheRead>::find_object_lt_or_eq_version::{{closure}} ... ``` The fix is to not overwrite an existing latest cache entry in this scenario, since it must already be in the correct state at the point at which the crash was triggered before.
1 parent 25e69bf commit 2ff990e

File tree

2 files changed

+22
-8
lines changed

2 files changed

+22
-8
lines changed

crates/sui-core/src/execution_cache/cache_types.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -188,7 +188,7 @@ const KEY_GENERATION_SIZE: usize = 1024 * 16;
188188

189189
impl<K, V> MonotonicCache<K, V>
190190
where
191-
K: Hash + Eq + Send + Sync + Copy + 'static,
191+
K: Hash + Eq + Send + Sync + Copy + std::fmt::Debug + 'static,
192192
V: IsNewer + Clone + Send + Sync + 'static,
193193
{
194194
pub fn new(cache_size: u64) -> Self {
@@ -304,7 +304,7 @@ where
304304

305305
// Ticket expiry should make this assert impossible.
306306
if entry.is_newer_than(&value) {
307-
debug_fatal!("entry is newer than value");
307+
debug_fatal!("entry is newer than value {:?}", key);
308308
} else {
309309
*entry = value;
310310
}

crates/sui-core/src/execution_cache/writeback_cache.rs

Lines changed: 20 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -173,6 +173,13 @@ impl LatestObjectCacheEntry {
173173
LatestObjectCacheEntry::NonExistent => None,
174174
}
175175
}
176+
177+
fn is_alive(&self) -> bool {
178+
match self {
179+
LatestObjectCacheEntry::Object(_, entry) => !entry.is_tombstone(),
180+
LatestObjectCacheEntry::NonExistent => false,
181+
}
182+
}
176183
}
177184

178185
impl IsNewer for LatestObjectCacheEntry {
@@ -1522,7 +1529,8 @@ impl ObjectCacheRead for WritebackCache {
15221529
// if we have the latest version cached, and it is within the bound, we are done
15231530
self.metrics
15241531
.record_cache_request("object_lt_or_eq_version", "object_by_id");
1525-
if let Some(latest) = self.cached.object_by_id_cache.get(&object_id) {
1532+
let latest_cache_entry = self.cached.object_by_id_cache.get(&object_id);
1533+
if let Some(latest) = &latest_cache_entry {
15261534
let latest = latest.lock();
15271535
match &*latest {
15281536
LatestObjectCacheEntry::Object(latest_version, object) => {
@@ -1628,12 +1636,18 @@ impl ObjectCacheRead for WritebackCache {
16281636
.find_object_lt_or_eq_version(object_id, version_bound)
16291637
.expect("db error")
16301638
}
1639+
1640+
// no object found in dirty set or db, object does not exist
1641+
// When this is called from a read api (i.e. not the execution path) it is
1642+
// possible that the object has been deleted and pruned. In this case,
1643+
// there would be no entry at all on disk, but we may have a tombstone in the
1644+
// cache
1645+
} else if let Some(latest_cache_entry) = latest_cache_entry {
1646+
// If there is a latest cache entry, it had better not be a live object!
1647+
assert!(!latest_cache_entry.lock().is_alive());
1648+
None
16311649
} else {
1632-
// no object found in dirty set or db, object does not exist
1633-
// When this is called from a read api (i.e. not the execution path) it is
1634-
// possible that the object has been deleted and pruned. In this case,
1635-
// there would be no entry at all on disk, but we may have a tombstone in the
1636-
// cache
1650+
// If there is no latest cache entry, we can insert one.
16371651
let highest = cached_entry.and_then(|c| c.get_highest());
16381652
assert!(highest.is_none() || highest.unwrap().1.is_tombstone());
16391653
self.cache_object_not_found(

0 commit comments

Comments
 (0)