SERVER-28352 Fix ticks increment in LogicalClock::reserveTicks

5 files changed, 66 insertions, 25 deletions

diff --git a/jstests/master_slave/master1.js b/jstests/master_slave/master1.js
index 2f5ba3e11eb..2292a061939 100644
--- a/jstests/master_slave/master1.js
+++ b/jstests/master_slave/master1.js
@@ -50,8 +50,6 @@ rt.stop(true);
 m = rt.start(true, null, true);
 assert.eq(op.ts.i, lastop().ts.i);
 
-assert.throws(function() {
-    am().save({});  // triggers fassert because ofclock skew
-});
+am().save({});
 
-assert.neq(0, rt.stop(true));  // fasserted
+assert.eq(0, rt.stop(true));
diff --git a/src/mongo/db/SConscript b/src/mongo/db/SConscript
index 68bd5121e5a..f54f36b421a 100644
--- a/src/mongo/db/SConscript
+++ b/src/mongo/db/SConscript
@@ -1009,6 +1009,7 @@ env.Library(
         'service_context',
         'signed_logical_time',
         'time_proof_service',
+        '$BUILD_DIR/mongo/util/clock_source_mock'
     ],
 )
 
diff --git a/src/mongo/db/logical_clock.cpp b/src/mongo/db/logical_clock.cpp
index 6f0e4c55574..4cdd3177d26 100644
--- a/src/mongo/db/logical_clock.cpp
+++ b/src/mongo/db/logical_clock.cpp
@@ -140,35 +140,48 @@ Status LogicalClock::signAndAdvanceClusterTime(LogicalTime newTime) {
     return _advanceClusterTime_inlock(std::move(newSignedTime));
 }
 
-LogicalTime LogicalClock::reserveTicks(uint64_t ticks) {
+LogicalTime LogicalClock::reserveTicks(uint64_t nTicks) {
 
-    invariant(ticks > 0);
+    invariant(nTicks > 0 && nTicks < (1U << 31));
 
     stdx::lock_guard<stdx::mutex> lock(_mutex);
 
+    LogicalTime clusterTime = _clusterTime.getTime();
+    LogicalTime nextClusterTime;
+
     const unsigned wallClockSecs =
         durationCount<Seconds>(_service->getFastClockSource()->now().toDurationSinceEpoch());
-    unsigned currentSecs = _clusterTime.getTime().asTimestamp().getSecs();
-    LogicalTime clusterTimestamp = _clusterTime.getTime();
+    unsigned clusterTimeSecs = clusterTime.asTimestamp().getSecs();
 
-    if (MONGO_unlikely(currentSecs < wallClockSecs)) {
-        clusterTimestamp = LogicalTime(Timestamp(wallClockSecs, 1));
-    } else {
-        clusterTimestamp.addTicks(1);
+    // Synchronize clusterTime with wall clock time, if clusterTime was behind in seconds.
+    if (clusterTimeSecs < wallClockSecs) {
+        clusterTime = LogicalTime(Timestamp(wallClockSecs, 0));
+    }
+    // If reserving 'nTicks' would force the cluster timestamp's increment field to exceed (2^31-1),
+    // overflow by moving to the next second. We use the signed integer maximum as an overflow point
+    // in order to preserve compatibility with potentially signed or unsigned integral Timestamp
+    // increment types. It is also unlikely to apply more than 2^31 oplog entries in the span of one
+    // second.
+    else if (clusterTime.asTimestamp().getInc() >= ((1U << 31) - nTicks)) {
+
+        log() << "Exceeded maximum allowable increment value within one second. Moving clusterTime "
+                 "forward to the next second.";
+
+        // Move time forward to the next second
+        clusterTime = LogicalTime(Timestamp(clusterTime.asTimestamp().getSecs() + 1, 0));
     }
-    auto currentTime = clusterTimestamp;
-    clusterTimestamp.addTicks(ticks - 1);
-
-    // Fail if time is not moving forward for 2**31 ticks
-    if (MONGO_unlikely(clusterTimestamp.asTimestamp().getSecs() > wallClockSecs) &&
-        clusterTimestamp.asTimestamp().getInc() >= 1U << 31) {
-        mongo::severe() << "clock skew detected, prev: " << wallClockSecs
-                        << " now: " << clusterTimestamp.asTimestamp().getSecs();
-        fassertFailed(17449);
+
+    // Save the next cluster time.
+    clusterTime.addTicks(1);
+    nextClusterTime = clusterTime;
+
+    // Add the rest of the requested ticks if needed.
+    if (nTicks > 1) {
+        clusterTime.addTicks(nTicks - 1);
     }
 
-    _clusterTime = _makeSignedLogicalTime(clusterTimestamp);
-    return currentTime;
+    _clusterTime = _makeSignedLogicalTime(clusterTime);
+    return nextClusterTime;
 }
 
 void LogicalClock::initClusterTimeFromTrustedSource(LogicalTime newTime) {
diff --git a/src/mongo/db/logical_clock.h b/src/mongo/db/logical_clock.h
index 06562c92719..0a4cb99f5fb 100644
--- a/src/mongo/db/logical_clock.h
+++ b/src/mongo/db/logical_clock.h
@@ -84,8 +84,9 @@ public:
     SignedLogicalTime getClusterTime();
 
     /**
-     * Returns the next  clusterTime value and provides the guarantee that the next reserveTicks
-     * call will return the value at least nTicks ticks in the future from the current clusterTime.
+     * Returns the next clusterTime value and provides a guarantee that any future call to
+     * reserveTicks() will return a value at least 'nTicks' ticks in the future from the current
+     * clusterTime.
      */
     LogicalTime reserveTicks(uint64_t nTicks);
 
diff --git a/src/mongo/db/logical_clock_test.cpp b/src/mongo/db/logical_clock_test.cpp
index 98c600a1421..f1b75133760 100644
--- a/src/mongo/db/logical_clock_test.cpp
+++ b/src/mongo/db/logical_clock_test.cpp
@@ -26,6 +26,8 @@
  *    it in the license file.
  */
 
+#define MONGO_LOG_DEFAULT_COMPONENT ::mongo::logger::LogComponent::kDefault
+
 #include "mongo/db/logical_clock.h"
 #include "mongo/bson/timestamp.h"
 #include "mongo/db/logical_time.h"
@@ -35,6 +37,8 @@
 #include "mongo/platform/basic.h"
 #include "mongo/stdx/memory.h"
 #include "mongo/unittest/unittest.h"
+#include "mongo/util/clock_source_mock.h"
+#include "mongo/util/log.h"
 
 namespace mongo {
 namespace {
@@ -49,6 +53,8 @@ protected:
         auto pTps = stdx::make_unique<TimeProofService>();
         _timeProofService = pTps.get();
         _clock = stdx::make_unique<LogicalClock>(_serviceContext.get(), std::move(pTps));
+        _serviceContext->setFastClockSource(
+                stdx::make_unique<SharedClockSourceAdapter>(_mockClockSource));
     }
 
     void tearDown() {
@@ -60,6 +66,14 @@ protected:
         return _clock.get();
     }
 
+    void setMockClockSourceTime(Date_t time) {
+        _mockClockSource->reset(time);
+    }
+
+    Date_t getMockClockSourceTime() {
+        return _mockClockSource->now();
+    }
+
     SignedLogicalTime makeSignedLogicalTime(LogicalTime logicalTime) {
         TimeProofService::Key key = {};
         return SignedLogicalTime(logicalTime, _timeProofService->getProof(logicalTime, key), 0);
@@ -73,6 +87,8 @@ protected:
 private:
     TimeProofService* _timeProofService;
     std::unique_ptr<ServiceContextNoop> _serviceContext;
+
+    std::shared_ptr<ClockSourceMock> _mockClockSource = std::make_shared<ClockSourceMock>();
     std::unique_ptr<LogicalClock> _clock;
 };
 
@@ -89,10 +105,16 @@ TEST_F(LogicalClockTestBase, roundtrip) {
 
 // Verify the reserve ticks functionality.
 TEST_F(LogicalClockTestBase, reserveTicks) {
+    // Set clock to a non-zero time, so we can verify wall clock synchronization.
+    setMockClockSourceTime(Date_t::fromMillisSinceEpoch(10 * 1000));
+
     auto t1 = getClock()->reserveTicks(1);
     auto t2(getClock()->getClusterTime());
     ASSERT_TRUE(t1 == t2.getTime());
 
+    // Make sure we synchronized with the wall clock.
+    ASSERT_TRUE(t2.getTime().asTimestamp().getSecs() == 10);
+
     auto t3 = getClock()->reserveTicks(1);
     t1.addTicks(1);
     ASSERT_TRUE(t3 == t1);
@@ -104,6 +126,12 @@ TEST_F(LogicalClockTestBase, reserveTicks) {
     t3 = getClock()->reserveTicks(1);
     t1.addTicks(100);
     ASSERT_TRUE(t3 == t1);
+
+    // Ensure overflow to a new second.
+    auto initTimeSecs = getClock()->getClusterTime().getTime().asTimestamp().getSecs();
+    getClock()->reserveTicks((1U << 31) - 1);
+    auto newTimeSecs = getClock()->getClusterTime().getTime().asTimestamp().getSecs();
+    ASSERT_TRUE(newTimeSecs == initTimeSecs + 1);
 }
 
 // Verify the advanceClusterTime functionality.