Enable the skip list corpus tests by default

driver_skip_list checks SkipList.cpp against ReferenceImpl, but its tests
were disabled, so the implementation diverged from the documented API
without anything noticing. The corpus tests pass in Debug and Release
builds, so run them like the other implementations' tests.

CMakeLists.txt

@@ -224,13 +224,11 @@ 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
+  # skip list tests, which cross-check SkipList.cpp against ReferenceImpl
-  if(0)
+  foreach(TEST ${CORPUS_TESTS})
-    foreach(TEST ${CORPUS_TESTS})
+    get_filename_component(hash ${TEST} NAME)
-      get_filename_component(hash ${TEST} NAME)
+    add_test(NAME skip_list_${hash} COMMAND driver_skip_list ${TEST})
-      add_test(NAME skip_list_${hash} COMMAND driver_skip_list ${TEST})
+  endforeach()
-    endforeach()
-  endif()
 
   # ad hoc testing
   add_executable(conflict_set_main ConflictSet.cpp)

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