From e292de5eb00d490da451de89ccaa730e3bebcca1 Mon Sep 17 00:00:00 2001
From: Brian Gough <bjg@network-theory.co.uk>
Date: Wed, 6 Apr 2016 17:00:16 +0100
Subject: [PATCH] fix to avoid ever appending permanent changes to expiring
 packs

---
 .../app/coffee/PackManager.coffee             | 17 ++++++++-
 .../PackManager/PackManagerTests.coffee       | 38 ++++++++++++++++++-
 2 files changed, 53 insertions(+), 2 deletions(-)

diff --git a/services/track-changes/app/coffee/PackManager.coffee b/services/track-changes/app/coffee/PackManager.coffee
index 0b04588804..b474d90605 100644
--- a/services/track-changes/app/coffee/PackManager.coffee
+++ b/services/track-changes/app/coffee/PackManager.coffee
@@ -50,6 +50,9 @@ module.exports = PackManager =
 	insertCompressedUpdates: (project_id, doc_id, lastUpdate, newUpdates, temporary, callback = (error) ->) ->
 		return callback() if newUpdates.length == 0
 
+		# never append permanent ops to a pack that will expire
+		lastUpdate = null if lastUpdate?.expiresAt? and not temporary
+
 		updatesToFlush = []
 		updatesRemaining = newUpdates.slice()
 
@@ -71,7 +74,19 @@ module.exports = PackManager =
 
 	flushCompressedUpdates:	(project_id, doc_id, lastUpdate, newUpdates, temporary, callback = (error) ->) ->
 		return callback() if newUpdates.length == 0
-		if lastUpdate? and not (temporary and ((Date.now() - lastUpdate.meta?.start_ts) > 1 * DAYS))
+
+		canAppend = false
+		# check if it is safe to append to an existing pack
+		if lastUpdate?
+			if not temporary and not lastUpdate.expiresAt?
+				# permanent pack appends to permanent pack
+				canAppend = true
+			age = Date.now() - lastUpdate.meta?.start_ts
+			if temporary and lastUpdate.expiresAt? and age < 1 * DAYS
+				# temporary pack appends to temporary pack if same day
+				canAppend = true
+
+		if canAppend
 			PackManager.appendUpdatesToExistingPack project_id, doc_id, lastUpdate, newUpdates, temporary, callback
 		else
 			PackManager.insertUpdatesIntoNewPack project_id, doc_id, newUpdates, temporary, callback
diff --git a/services/track-changes/test/unit/coffee/PackManager/PackManagerTests.coffee b/services/track-changes/test/unit/coffee/PackManager/PackManagerTests.coffee
index 1d929722bd..bd2244517f 100644
--- a/services/track-changes/test/unit/coffee/PackManager/PackManagerTests.coffee
+++ b/services/track-changes/test/unit/coffee/PackManager/PackManagerTests.coffee
@@ -171,7 +171,7 @@ describe "PackManager", ->
 					it "should call the callback", ->
 						@callback.called.should.equal true
 
-		describe "when there is a recent previous update in mongo", ->
+		describe "when there is a recent previous update in mongo that expires", ->
 			beforeEach ->
 				@lastUpdate = {
 					_id: "12345"
@@ -181,6 +181,7 @@ describe "PackManager", ->
 					]
 					n : 2
 					sz : 100
+					meta: {start_ts: Date.now() - 6 * 3600 * 1000}
 					expiresAt: new Date(Date.now())
 				}
 
@@ -202,6 +203,41 @@ describe "PackManager", ->
 					@callback.called.should.equal true
 
 
+		describe "when there is a recent previous update in mongo that expires", ->
+			beforeEach ->
+				@PackManager.updateIndex = sinon.stub().callsArg(2)
+
+				@lastUpdate = {
+					_id: "12345"
+					pack: [
+						{ op: "op-1", meta: "meta-1", v: 1},
+						{ op: "op-2", meta: "meta-2", v: 2}
+					]
+					n : 2
+					sz : 100
+					meta: {start_ts: Date.now() - 6 * 3600 * 1000}
+					expiresAt: new Date(Date.now())
+				}
+
+				@PackManager.flushCompressedUpdates @project_id, @doc_id, @lastUpdate, @newUpdates, false, @callback
+
+			describe "for a small update that will not expire", ->
+				it "should insert the update into mongo", ->
+					@db.docHistory.save.calledWithMatch({
+						pack: @newUpdates,
+						project_id: ObjectId(@project_id),
+						doc_id: ObjectId(@doc_id)
+						n: @newUpdates.length
+						v: @newUpdates[0].v
+						v_end: @newUpdates[@newUpdates.length-1].v
+					}).should.equal true
+
+				it "should not set any expiry time", ->
+					@db.docHistory.save.neverCalledWithMatch(sinon.match.has("expiresAt")).should.equal true
+
+				it "should call the callback", ->
+					@callback.called.should.equal true
+
 		describe "when there is an old previous update in mongo", ->
 			beforeEach ->
 				@lastUpdate = {