Merge pull request #709 from share/fix/fetch-snapshot-by-timestamp-missing-ops

🐛 Serve current snapshot when fetching by timestamp after latest op

Author: alecgibson

lib/backend.js

@@ -881,29 +881,38 @@ Backend.prototype._fetchSnapshotByTimestamp = function(collection, id, timestamp
   var from = 0;
   var to = null;
 
-  var shouldGetLatestSnapshot = timestamp === null;
-  if (shouldGetLatestSnapshot) {
-    return backend.db.getSnapshot(collection, id, null, null, function(error, snapshot) {
-      if (error) return callback(error);
-
-      callback(null, snapshot);
-    });
-  }
-
-  milestoneDb.getMilestoneSnapshotAtOrBeforeTime(collection, id, timestamp, function(error, snapshot) {
+  // Always fetch the current snapshot first. We request its metadata so that we
+  // can read its mtime, which lets us serve the current snapshot directly when
+  // the requested timestamp is after it. This avoids replaying ops when they
+  // aren't needed, and - crucially - still works when older ops have been
+  // deleted/TTLed and the current version can no longer be rebuilt from ops.
+  db.getSnapshot(collection, id, null, {metadata: true}, function(error, currentSnapshot) {
     if (error) return callback(error);
-    milestoneSnapshot = snapshot;
-    if (snapshot) from = snapshot.v;
 
-    milestoneDb.getMilestoneSnapshotAtOrAfterTime(collection, id, timestamp, function(error, snapshot) {
+    var mtime = currentSnapshot.m && currentSnapshot.m.mtime;
+    var shouldGetLatestSnapshot = timestamp === null || (mtime != null && timestamp > mtime);
+    if (shouldGetLatestSnapshot) {
+      // Strip the metadata that we only fetched in order to compare the mtime,
+      // so that the returned snapshot is consistent with the op-replayed path.
+      currentSnapshot.m = null;
+      return callback(null, currentSnapshot);
+    }
+
+    milestoneDb.getMilestoneSnapshotAtOrBeforeTime(collection, id, timestamp, function(error, snapshot) {
       if (error) return callback(error);
-      if (snapshot) to = snapshot.v;
+      milestoneSnapshot = snapshot;
+      if (snapshot) from = snapshot.v;
 
-      var options = {metadata: true};
-      db.getOps(collection, id, from, to, options, function(error, ops) {
+      milestoneDb.getMilestoneSnapshotAtOrAfterTime(collection, id, timestamp, function(error, snapshot) {
         if (error) return callback(error);
-        filterOpsInPlaceBeforeTimestamp(ops, timestamp);
-        backend._buildSnapshotFromOps(id, milestoneSnapshot, ops, callback);
+        if (snapshot) to = snapshot.v;
+
+        var options = {metadata: true};
+        db.getOps(collection, id, from, to, options, function(error, ops) {
+          if (error) return callback(error);
+          filterOpsInPlaceBeforeTimestamp(ops, timestamp);
+          backend._buildSnapshotFromOps(id, milestoneSnapshot, ops, callback);
+        });
       });
     });
   });

test/client/snapshot-timestamp-request.js

@@ -139,6 +139,21 @@ describe('SnapshotTimestampRequest', function() {
       ], done);
     });
 
+    it('fetches the current snapshot when ops are missing and the timestamp is after the latest op', function(done) {
+      var connection = backend.connect();
+      async.series([
+        // Simulate ops having been deleted/TTLed so the snapshot can't be rebuilt from ops.
+        backend.db.deleteOps.bind(backend.db, 'books', 'time-machine', null, null, null),
+        function(next) {
+          connection.fetchSnapshotByTimestamp('books', 'time-machine', day4, function(error, snapshot) {
+            if (error) return next(error);
+            expect(snapshot).to.eql(v3);
+            next();
+          });
+        }
+      ], done);
+    });
+
     it('fetches the most recent version when not specifying a timestamp', function(done) {
       var connection = backend.connect();
       async.waterfall([