Review feedback

services/document-updater/app/coffee/SnapshotManager.coffee

@@ -1,17 +1,20 @@
 {db, ObjectId} = require "./mongojs"
 
 module.exports = SnapshotManager =
-  recordSnapshot: (project_id, doc_id, version, lines, ranges, callback) ->
+  recordSnapshot: (project_id, doc_id, version, pathname, lines, ranges, callback) ->
     try
       project_id = ObjectId(project_id)
       doc_id = ObjectId(doc_id)
     catch error
       return callback(error)
     db.docSnapshots.insert {
-      project_id, doc_id, version, lines
+      project_id, doc_id, version, lines, pathname,
       ranges: SnapshotManager.jsonRangesToMongo(ranges),
       ts: new Date()
     }, callback
+    # Suggested indexes:
+    #   db.docSnapshots.createIndex({project_id:1, doc_id:1})
+    #   db.docSnapshots.createIndex({ts:1},{expiresAfterSeconds: 30*24*3600)) # expires after 30 days
 
   jsonRangesToMongo: (ranges) ->
     return null if !ranges?

services/document-updater/app/coffee/UpdateManager.coffee

@@ -85,17 +85,24 @@ module.exports = UpdateManager =
 					UpdateManager._addProjectHistoryMetadataToOps(appliedOps, pathname, projectHistoryId, lines)
 					profile.log("RangesManager.applyUpdate")
 					return callback(error) if error?
-					if ranges_were_collapsed
-						logger.log {project_id, doc_id, previousVersion, lines, ranges, update}, "update collapsed some ranges, snapshotting previous content"
-						SnapshotManager.recordSnapshot project_id, doc_id, previousVersion, lines, ranges, (error) ->
-							if error?
-								logger.error {err: error, project_id, doc_id, version, lines, ranges}, "error recording snapshot"
 					RedisManager.updateDocument project_id, doc_id, updatedDocLines, version, appliedOps, new_ranges, (error, doc_ops_length, project_ops_length) ->
 						profile.log("RedisManager.updateDocument")
 						return callback(error) if error?
 						HistoryManager.recordAndFlushHistoryOps project_id, doc_id, appliedOps, doc_ops_length, project_ops_length, (error) ->
 							profile.log("recordAndFlushHistoryOps")
-							callback(error)
+							return callback(error) if error?
+							if ranges_were_collapsed
+								logger.log {project_id, doc_id, previousVersion, lines, ranges, update}, "update collapsed some ranges, snapshotting previous content"
+								# Do this last, since it's a mongo call, and so potentially longest running
+								# If it overruns the lock, it's ok, since all of our redis work is done
+								SnapshotManager.recordSnapshot project_id, doc_id, previousVersion, pathname, lines, ranges, (error) ->
+									if error?
+										logger.error {err: error, project_id, doc_id, version, lines, ranges}, "error recording snapshot"
+										return callback(error)
+									else
+										callback()
+							else
+								callback()
 
 	lockUpdatesAndDo: (method, project_id, doc_id, args..., callback) ->
 		profile = new Profiler("lockUpdatesAndDo", {project_id, doc_id})

services/document-updater/test/acceptance/coffee/RangesTests.coffee

@@ -311,7 +311,7 @@ describe "Ranges", ->
 				expect(ranges.changes).to.be.undefined
 				done()
 
-	describe.only "deleting text surrounding a comment", ->
+	describe "deleting text surrounding a comment", ->
 		before (done) ->
 			@project_id = DocUpdaterClient.randomId()
 			@user_id = DocUpdaterClient.randomId()

services/document-updater/test/unit/coffee/UpdateManager/UpdateManagerTests.coffee

@@ -252,6 +252,7 @@ describe "UpdateManager", ->
 						@project_id,
 						@doc_id,
 						@version,
+						@pathname,
 						@lines,
 						@ranges
 					)