Remove objects from sync queue if missing from response

While objects in the sync queue that fail to save should remain in the
queue, objects that just don't exist remotely need to be removed, or
else they'll be retried forever.

chrome/content/zotero/xpcom/sync/syncEngine.js

@@ -436,6 +436,7 @@ Zotero.Sync.Data.Engine.prototype._downloadUpdatedObjects = Zotero.Promise.corou
 	}
 	
 	keys.push(...queuedKeys);
+	keys = Zotero.Utilities.arrayUnique(keys);
 	
 	if (!keys.length) {
 		Zotero.debug(`No ${objectTypePlural} to download`);
@@ -455,6 +456,7 @@ Zotero.Sync.Data.Engine.prototype._downloadUpdatedObjects = Zotero.Promise.corou
 Zotero.Sync.Data.Engine.prototype._downloadObjects = Zotero.Promise.coroutine(function* (objectType, keys) {
 	var objectTypePlural = Zotero.DataObjectUtilities.getObjectTypePlural(objectType);
 	
+	var remainingKeys = [...keys];
 	var lastLength = keys.length;
 	var objectData = {};
 	keys.forEach(key => objectData[key] = null);
@@ -462,8 +464,6 @@ Zotero.Sync.Data.Engine.prototype._downloadObjects = Zotero.Promise.coroutine(fu
 	while (true) {
 		this._failedCheck();
 		
-		let lastError = false;
-		
 		// Get data we've downloaded in a previous loop but failed to process
 		var json = [];
 		let keysToDownload = [];
@@ -491,7 +491,7 @@ Zotero.Sync.Data.Engine.prototype._downloadObjects = Zotero.Promise.coroutine(fu
 			"Downloading "
 			+ (keysToDownload.length == 1
 				? "1 " + objectType
-				: Zotero.Utilities.numberFormat(keys.length, 0) + " " + objectTypePlural)
+				: Zotero.Utilities.numberFormat(remainingKeys.length, 0) + " " + objectTypePlural)
 			+ " in " + this.library.name
 		);
 		
@@ -510,7 +510,6 @@ Zotero.Sync.Data.Engine.prototype._downloadObjects = Zotero.Promise.coroutine(fu
 				
 				if (!Array.isArray(batch)) {
 					this.failed = batch;
-					lastError = batch;
 					return;
 				}
 				
@@ -564,7 +563,7 @@ Zotero.Sync.Data.Engine.prototype._downloadObjects = Zotero.Promise.coroutine(fu
 						conflictResults.push(x);
 					}
 				});
-				keys = Zotero.Utilities.arrayDiff(keys, processedKeys);
+				remainingKeys = Zotero.Utilities.arrayDiff(remainingKeys, processedKeys);
 				conflicts.push(...conflictResults);
 			}.bind(this)),
 			{
@@ -572,12 +571,28 @@ Zotero.Sync.Data.Engine.prototype._downloadObjects = Zotero.Promise.coroutine(fu
 			}
 		);
 		
-		if (!keys.length || keys.length == lastLength) {
+		this._failedCheck();
+		
+		// If all requests were successful, such that we had a chance to see all keys, remove keys we
+		// didn't see from the sync queue so they don't keep being retried forever
+		if (!this.failed) {
+			let missingKeys = keys.filter(key => objectData[key] === null);
+			if (missingKeys.length) {
+				Zotero.debug(`Removing ${missingKeys.length} missing `
+					+ Zotero.Utilities.pluralize(missingKeys.length, [objectType, objectTypePlural])
+					+ " from sync queue");
+				yield Zotero.Sync.Data.Local.removeObjectsFromSyncQueue(objectType, this.libraryID, missingKeys);
+				remainingKeys = Zotero.Utilities.arrayDiff(remainingKeys, missingKeys);
+			}
+		}
+		
+		if (!remainingKeys.length || remainingKeys.length == lastLength) {
 			// Add failed objects to sync queue
-			let failedKeys = Object.keys(objectData).filter(key => objectData[key])
+			let failedKeys = keys.filter(key => objectData[key]);
 			if (failedKeys.length) {
-				let objDesc = `${failedKeys.length == 1 ? objectType : objectTypePlural}`;
+				Zotero.debug(`Queueing ${failedKeys.length} failed `
-				Zotero.debug(`Queueing ${failedKeys.length} failed ${objDesc} for later`, 2);
+					+ Zotero.Utilities.pluralize(failedKeys.length, [objectType, objectTypePlural])
+					+ " for later", 2);
 				yield Zotero.Sync.Data.Local.addObjectsToSyncQueue(
 					objectType, this.libraryID, failedKeys
 				);
@@ -598,10 +613,10 @@ Zotero.Sync.Data.Engine.prototype._downloadObjects = Zotero.Promise.coroutine(fu
 			break;
 		}
 		
-		lastLength = keys.length;
+		lastLength = remainingKeys.length;
 		
-		var remainingObjectDesc = `${keys.length == 1 ? objectType : objectTypePlural}`;
+		Zotero.debug(`Retrying ${remainingKeys.length} remaining `
-		Zotero.debug(`Retrying ${keys.length} remaining ${remainingObjectDesc}`);
+			+ Zotero.Utilities.pluralize(remainingKeys, [objectType, objectTypePlural]));
 	}
 	
 	// Show conflict resolution window

chrome/content/zotero/xpcom/sync/syncLocal.js

@@ -1692,10 +1692,10 @@ Zotero.Sync.Data.Local = {
 	
 	
 	getObjectsToTryFromSyncQueue: Zotero.Promise.coroutine(function* (objectType, libraryID) {
+		var syncObjectTypeID = Zotero.Sync.Data.Utilities.getSyncObjectTypeID(objectType);
 		var rows = yield Zotero.DB.queryAsync(
-			"SELECT key, lastCheck, tries FROM syncQueue WHERE libraryID=? AND "
+			"SELECT key, lastCheck, tries FROM syncQueue WHERE libraryID=? AND syncObjectTypeID=?",
-				+ "syncObjectTypeID IN (SELECT syncObjectTypeID FROM syncObjectTypes WHERE name=?)",
+			[libraryID, syncObjectTypeID]
-			[libraryID, objectType]
 		);
 		var keysToTry = [];
 		for (let row of rows) {

test/tests/syncEngineTest.js

@@ -2056,6 +2056,88 @@ describe("Zotero.Sync.Data.Engine", function () {
 	});
 	
 	
+	describe("#_downloadUpdatedObjects()", function () {
+		it("should include objects in sync queue", function* () {
+			({ engine, client, caller } = yield setup());
+			
+			var libraryID = Zotero.Libraries.userLibraryID;
+			var objectType = 'collection';
+			var objectTypeID = Zotero.Sync.Data.Utilities.getSyncObjectTypeID(objectType);
+			yield Zotero.Sync.Data.Local.addObjectsToSyncQueue(
+				objectType, libraryID, ["BBBBBBBB", "CCCCCCCC"]
+			);
+			yield Zotero.DB.queryAsync(
+				"UPDATE syncQueue SET lastCheck=lastCheck-3600 "
+					+ "WHERE syncObjectTypeID=? AND libraryID=? AND key IN (?, ?)",
+				[objectTypeID, libraryID, 'BBBBBBBB', 'CCCCCCCC']
+			);
+			
+			var headers = {
+				"Last-Modified-Version": 5
+			};
+			setResponse({
+				method: "GET",
+				url: "users/1/collections?format=versions&since=1",
+				status: 200,
+				headers,
+				json: {
+					AAAAAAAA: 5,
+					BBBBBBBB: 5
+				}
+			});
+			
+			var stub = sinon.stub(engine, "_downloadObjects");
+			
+			yield engine._downloadUpdatedObjects(objectType, 1, 5);
+			
+			assert.ok(stub.calledWith("collection", ["AAAAAAAA", "BBBBBBBB", "CCCCCCCC"]));
+			stub.restore();
+		});
+	});
+	
+	
+	describe("#_downloadObjects()", function () {
+		it("should remove object from sync queue if missing from response", function* () {
+			({ engine, client, caller } = yield setup({
+				stopOnError: false
+			}));
+			var libraryID = Zotero.Libraries.userLibraryID;
+			var objectType = 'collection';
+			var objectTypeID = Zotero.Sync.Data.Utilities.getSyncObjectTypeID(objectType);
+			yield Zotero.Sync.Data.Local.addObjectsToSyncQueue(
+				objectType, libraryID, ["BBBBBBBB", "CCCCCCCC"]
+			);
+			
+			var headers = {
+				"Last-Modified-Version": 5
+			};
+			setResponse({
+				method: "GET",
+				url: "users/1/collections?format=json&collectionKey=AAAAAAAA%2CBBBBBBBB%2CCCCCCCCC",
+				status: 200,
+				headers,
+				json: [
+					makeCollectionJSON({
+						key: "AAAAAAAA",
+						version: 5,
+						name: "A"
+					}),
+					makeCollectionJSON({
+						key: "BBBBBBBB",
+						version: 5
+						// Missing 'name', which causes a save error
+					})
+				]
+			});
+			yield engine._downloadObjects(objectType, ["AAAAAAAA", "BBBBBBBB", "CCCCCCCC"]);
+			
+			// Missing object should have been removed, but invalid object should remain
+			var keys = yield Zotero.Sync.Data.Local.getObjectsFromSyncQueue(objectType, libraryID);
+			assert.sameMembers(keys, ['BBBBBBBB']);
+		});
+	});
+	
+	
 	describe("#_startUpload()", function () {
 		it("shouldn't upload unsynced objects if present in sync queue", function* () {
 			({ engine, client, caller } = yield setup());