Merge pull request 'SkipList: don't lower oldestVersion in setOldestVersion' (#92) from weaselbot/conflict-set:weaselbot/issue-87 into main

Reviewed-on: #92

CMakeLists.txt

@@ -182,6 +182,8 @@ endif()
 
 include(CTest)
 
+option(TEST_SKIP_LIST "Run the slow skip list cross-check tests" OFF)
+
 # disable tests if this is being used through e.g. FetchContent
 if(CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR AND BUILD_TESTING)
 
@@ -224,8 +226,9 @@ if(CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR AND BUILD_TESTING)
   target_compile_options(driver_skip_list PRIVATE ${TEST_FLAGS})
   target_link_libraries(driver_skip_list PRIVATE skip_list)
 
-  # enable to test skip list
+  # Optional cross-check of SkipList.cpp against ReferenceImpl. Opt-in via
-  if(0)
+  # -DTEST_SKIP_LIST=ON, since these are slow.
+  if(TEST_SKIP_LIST)
     foreach(TEST ${CORPUS_TESTS})
       get_filename_component(hash ${TEST} NAME)
       add_test(NAME skip_list_${hash} COMMAND driver_skip_list ${TEST})

SkipList.cpp

@@ -819,10 +819,17 @@ struct __attribute__((visibility("hidden"))) ConflictSet::Impl {
   }
 
   void setOldestVersion(int64_t oldestVersion) {
+    // oldestVersion becomes the max of its previous value and the argument, so
+    // an older version is a no-op. We still fall through in the equal case so
+    // that garbage collection can make progress without advancing the oldest
+    // version.
+    if (oldestVersion < this->oldestVersion) {
+      return;
+    }
+
     // This isn't 100% accurate. It overcounts if you hit the end
     gc_iterations_total.add(keyUpdates);
 
-    assert(oldestVersion >= this->oldestVersion);
     this->oldestVersion = oldestVersion;
     SkipList::Finger finger;
     int temp;

test_conflict_set.py

@@ -68,6 +68,35 @@ def test_hash_table_getBytes():
         assert cs.check(read(0, b"key")) == [Result.CONFLICT]
 
 
+def test_setOldestVersion_does_not_decrease():
+    # Regression test for issue #87: `setOldestVersion` sets oldestVersion to
+    # the maximum of its previous value and its argument, so passing an older
+    # version is valid input and must be a no-op. skip_list used to assert in
+    # Debug builds and, in Release builds, lower its oldest version, which
+    # resurrected a window of versions that callers were told resulted in
+    # TooOld.
+    with DebugConflictSet() as cs:
+        cs.addWrites(5, write(b"k"))
+        cs.setOldestVersion(5)
+        # Valid: `2 >= 0` and `2 <= newestVersion == 5`. oldestVersion stays 5,
+        # so 3 and 4 are still TooOld.
+        cs.setOldestVersion(2)
+        assert cs.check(read(3, b"k")) == [Result.TOO_OLD]
+        assert cs.check(read(4, b"k")) == [Result.TOO_OLD]
+        assert cs.check(read(5, b"k")) == [Result.COMMIT]
+
+    # Same, but with the entries at the old oldestVersion already garbage
+    # collected, so lowering oldestVersion would report Commit for a version
+    # that the caller was told resulted in TooOld.
+    with DebugConflictSet() as cs:
+        cs.addWrites(1, write(b"a"))
+        cs.addWrites(2)  # No writes; advances newestVersion to 2
+        cs.setOldestVersion(2)  # Garbage collects the write at version 1
+        cs.setOldestVersion(1)
+        assert cs.check(read(1, b"a")) == [Result.TOO_OLD]
+        assert cs.check(read(2, b"a")) == [Result.COMMIT]
+
+
 def test_hash_table_setOldestVersion_empties_map():
     # Regression test for issue #78: setOldestVersion must terminate when
     # its garbage-collection pass erases every entry in the map. Each write