From df38f4ded7813fa2f9b4a6f74a731954296df68d Mon Sep 17 00:00:00 2001
From: Dan Stillman <dstillman@zotero.org>
Date: Sun, 10 Dec 2017 03:26:16 -0500
Subject: [PATCH] Avoid upload retry loops

- Don't try uploading an object more than 5 times
- Don't retry a child item if the parent item failed too
---
 .../content/zotero/xpcom/sync/syncEngine.js   | 24 +++++--
 test/tests/syncEngineTest.js                  | 64 +++++++++++++++++++
 2 files changed, 84 insertions(+), 4 deletions(-)

diff --git a/chrome/content/zotero/xpcom/sync/syncEngine.js b/chrome/content/zotero/xpcom/sync/syncEngine.js
index c59018bd6b..ced55ca7fa 100644
--- a/chrome/content/zotero/xpcom/sync/syncEngine.js
+++ b/chrome/content/zotero/xpcom/sync/syncEngine.js
@@ -51,6 +51,7 @@ Zotero.Sync.Data.Engine = function (options) {
 	this.libraryTypeID = this.library.libraryTypeID;
 	this.uploadBatchSize = 25;
 	this.uploadDeletionBatchSize = 50;
+	this.maxUploadTries = 5;
 	
 	this.failed = false;
 	this.failedItems = [];
@@ -1172,8 +1173,8 @@ Zotero.Sync.Data.Engine.prototype._uploadObjects = Zotero.Promise.coroutine(func
 			let numSkipped = 0;
 			for (let i = 0; i < queue.length && i < this.uploadBatchSize; i++) {
 				let o = queue[i];
-				// Skip requests that failed with 4xx
-				if (o.failed) {
+				// Skip requests that failed with 4xx or that have been retried too many times
+				if (o.failed || o.tries >= this.maxUploadTries) {
 					numSkipped++;
 					continue;
 				}
@@ -1198,6 +1199,7 @@ Zotero.Sync.Data.Engine.prototype._uploadObjects = Zotero.Promise.coroutine(func
 			
 			// No more non-failed requests
 			if (!batch.length) {
+				Zotero.debug(`No more ${objectTypePlural} to upload`);
 				break;
 			}
 			
@@ -1334,10 +1336,24 @@ Zotero.Sync.Data.Engine.prototype._uploadObjects = Zotero.Promise.coroutine(func
 				}
 				batch[index].tries++;
 				// Mark 400 errors as permanently failed
-				if (e.code >= 400 && e.code < 500) {
+				if (e.code < 500) {
 					batch[index].failed = true;
 				}
-				// 500 errors should stay in queue and be retried
+				// 500 errors should stay in queue and be retried, unless a dependency also failed
+				else if (objectType == 'item') {
+					// Check parent item
+					let parentItem = batch[index].json.parentItem;
+					if (parentItem) {
+						for (let i in batch) {
+							if (i == index) break;
+							let o = batch[i];
+							if (o.failed && o.json.key) {
+								Zotero.debug(`Not retrying child of failed parent ${parentItem}`);
+								batch[index].failed = true;
+							}
+						}
+					}
+				}
 			}
 			
 			// Add failed objects back to end of queue
diff --git a/test/tests/syncEngineTest.js b/test/tests/syncEngineTest.js
index d1e5aa32cd..720be9b585 100644
--- a/test/tests/syncEngineTest.js
+++ b/test/tests/syncEngineTest.js
@@ -2960,6 +2960,70 @@ describe("Zotero.Sync.Data.Engine", function () {
 			assert.equal(result, engine.UPLOAD_RESULT_SUCCESS);
 			assert.equal(called, 2);
 		});
+		
+		
+		it("shouldn't retry failed child item if parent item failed during this sync", async function () {
+			({ engine, client, caller } = await setup({
+				stopOnError: false
+			}));
+			
+			var library = Zotero.Libraries.userLibrary;
+			var libraryID = library.id;
+			var libraryVersion = 5;
+			library.libraryVersion = libraryVersion;
+			await library.saveTx();
+			
+			var item1 = await createDataObject('item');
+			var item1JSON = item1.toResponseJSON();
+			var tag = "A".repeat(300);
+			var item2 = await createDataObject('item', { tags: [{ tag }] });
+			var note = await createDataObject('item', { itemType: 'note', parentID: item2.id });
+			
+			var called = 0;
+			server.respond(function (req) {
+				let requestJSON = JSON.parse(req.requestBody);
+				if (called == 0) {
+					assert.lengthOf(requestJSON, 3);
+					assert.equal(requestJSON[0].key, item1.key);
+					assert.equal(requestJSON[1].key, item2.key);
+					assert.equal(requestJSON[2].key, note.key);
+					req.respond(
+						200,
+						{
+							"Last-Modified-Version": ++libraryVersion
+						},
+						JSON.stringify({
+							successful: {
+								"0": Object.assign(item1JSON, { version: libraryVersion })
+							},
+							unchanged: {},
+							failed: {
+								1: {
+									code: 413,
+									message: `Tag '${"A".repeat(50)}…' too long`,
+									data: {
+										tag
+									}
+								},
+								// Normally this would retry, but that might result in a 409
+								// without the parent
+								2: {
+									code: 500,
+									message: `An error occurred`
+								}
+							}
+						})
+					);
+				}
+				called++;
+			});
+			
+			var spy = sinon.spy(engine, "onError");
+			var result = await engine._startUpload();
+			assert.equal(result, engine.UPLOAD_RESULT_SUCCESS);
+			assert.equal(called, 1);
+			assert.equal(spy.callCount, 2);
+		});
 	});