SkipList: don't lower oldestVersion in setOldestVersion

setOldestVersion is documented to set oldestVersion to the maximum of its
previous value and its argument, so passing a version older than the
current oldestVersion is valid input. SkipList.cpp instead asserted that
the argument was monotonically non-decreasing, aborting Debug builds, and
with NDEBUG it lowered oldestVersion. That resurrected a window of
versions that callers had been told result in TooOld: reads in that
window were evaluated against entries that may already have been garbage
collected, so they could report Commit or Conflict instead.

Leave oldestVersion alone when the argument isn't newer, mirroring
HashTable.cpp. Falling through on equality keeps garbage collection
making progress without advancing the oldest version.

Add a scripted regression test, which bisimulates against the radix tree.

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