Stop file upload queue after low-quota errors

We weren't making actual upload requests after a quota error if the file
would exceed the quota, but we were still going through all attachments
to upload, which in some cases involves making stat() calls. We now just
stop the queue immediately after a quota error or when starting a new
background sync after a previous quota error.

Closes #1255

chrome/content/zotero/xpcom/storage/storageEngine.js

@@ -292,10 +292,19 @@ Zotero.Sync.Storage.Engine.prototype.start = Zotero.Promise.coroutine(function*
 })
 
 
-Zotero.Sync.Storage.Engine.prototype.stop = function () {
-	Zotero.debug("Stopping file sync for " + this.library.name);
-	for (let type in this.queues) {
-		this.queues[type].stop();
+/**
+ * @param {String} [queueToStop] - 'upload' or 'download'; if not specified, stop all queues
+ */
+Zotero.Sync.Storage.Engine.prototype.stop = function (queueToStop) {
+	if (queueToStop) {
+		Zotero.debug(`Stopping file sync ${queueToStop} queue for ` + this.library.name);
+		this.queues[queueToStop].stop();
+	}
+	else {
+		Zotero.debug("Stopping file sync for " + this.library.name);
+		for (let type in this.queues) {
+			this.queues[type].stop();
+		}
 	}
 }
 
@@ -330,9 +339,10 @@ Zotero.Sync.Storage.Engine.prototype.queueItem = Zotero.Promise.coroutine(functi
 	this.queues[type].add(() => {
 		var request = new Zotero.Sync.Storage.Request({
 			type,
+			engine: this,
 			libraryID: this.libraryID,
 			name: item.libraryKey,
-			onStart: request => this.controller[fn](request),
+			onStart: request => this.controller[fn](request, this),
 			onStop: () => {
 				this.requestsRemaining--;
 				this.onProgress(this.numRequests - this.requestsRemaining, this.numRequests);

chrome/content/zotero/xpcom/storage/storageLocal.js

@@ -9,6 +9,10 @@ Zotero.Sync.Storage.Local = {
 	SYNC_STATE_FORCE_DOWNLOAD: 4,
 	SYNC_STATE_IN_CONFLICT: 5,
 	
+	// If last-known remaining storage is below this number of megabytes, skip further upload
+	// attempts in various situations
+	STORAGE_REMAINING_MINIMUM: 5,
+	
 	lastFullFileCheck: {},
 	uploadCheckFiles: [],
 	storageRemainingForLibrary: new Map(),

chrome/content/zotero/xpcom/storage/storageRequest.js

@@ -28,6 +28,7 @@
  * Transfer request for storage sync
  *
  * @param {Object} options
+ * @param {Zotero.Sync.Storage.Engine} options.engine
  * @param {String} options.type
  * @param {Integer} options.libraryID
  * @param {String} options.name - Identifier for request (e.g., "[libraryID]/[key]")
@@ -36,16 +37,18 @@
  * @param {Function|Function[]} [options.onStop]
  */
 Zotero.Sync.Storage.Request = function (options) {
+	if (!options.engine) throw new Error("engine must be provided");
 	if (!options.type) throw new Error("type must be provided");
 	if (!options.libraryID) throw new Error("libraryID must be provided");
 	if (!options.name) throw new Error("name must be provided");
-	['type', 'libraryID', 'name'].forEach(x => this[x] = options[x]);
+	['engine', 'type', 'libraryID', 'name'].forEach(x => this[x] = options[x]);
 	
 	Zotero.debug(`Initializing ${this.type} request ${this.name}`);
 	
 	this.callbacks = ['onStart', 'onProgress', 'onStop'];
 	
 	this.Type = Zotero.Utilities.capitalize(this.type);
+	this.engine = options.engine;
 	this.channel = null;
 	this.queue = null;
 	this.progress = 0;

chrome/content/zotero/xpcom/storage/zfs.js

@@ -284,6 +284,11 @@ Zotero.Sync.Storage.Mode.ZFS.prototype = {
 				}
 			}
 			if (skip) {
+				// Stop trying to upload files if there's very little storage remaining
+				if (remaining < Zotero.Sync.Storage.Local.STORAGE_REMAINING_MINIMUM) {
+					Zotero.debug(`${remaining} MB remaining in storage -- skipping further uploads`);
+					request.engine.stop('upload');
+				}
 				throw yield this._getQuotaError(item);
 			}
 		}
@@ -294,7 +299,22 @@ Zotero.Sync.Storage.Mode.ZFS.prototype = {
 				return new Zotero.Sync.Storage.Result;
 			}
 		}
-		return this._processUploadFile(request);
+		
+		try {
+			return yield this._processUploadFile(request);
+		}
+		catch (e) {
+			// Stop trying to upload files if we hit a quota error and there's very little space
+			// remaining. If there's more space, we keep going, because it might just be a big file.
+			if (e.error == Zotero.Error.ERROR_ZFS_OVER_QUOTA) {
+				let remaining = Zotero.Sync.Storage.Local.storageRemainingForLibrary.get(item.libraryID);
+				if (remaining < Zotero.Sync.Storage.Local.STORAGE_REMAINING_MINIMUM) {
+					Zotero.debug(`${remaining} MB remaining in storage -- skipping further uploads`);
+					request.engine.stop('upload');
+				}
+			}
+			throw e;
+		}
 	}),
 	
 	
@@ -607,7 +627,7 @@ Zotero.Sync.Storage.Mode.ZFS.prototype = {
 			}
 			
 			// Store the remaining space so that we can skip files bigger than that until the next
-			// manual sync
+			// manual sync. Values are in megabytes.
 			let usage = req.getResponseHeader('Zotero-Storage-Usage');
 			let quota = req.getResponseHeader('Zotero-Storage-Quota');
 			Zotero.Sync.Storage.Local.storageRemainingForLibrary.set(item.libraryID, quota - usage);

test/tests/zfsTest.js

@@ -74,9 +74,10 @@ describe("Zotero.Sync.Storage.Mode.ZFS", function () {
 	
 	var setup = Zotero.Promise.coroutine(function* (options = {}) {
 		Components.utils.import("resource://zotero/concurrentCaller.js");
+		var stopOnError = options.stopOnError !== undefined ? options.stopOnError : true;
 		var caller = new ConcurrentCaller(1);
 		caller.setLogger(msg => Zotero.debug(msg));
-		caller.stopOnError = true;
+		caller.stopOnError = stopOnError;
 		
 		Components.utils.import("resource://zotero/config.js");
 		var client = new Zotero.Sync.APIClient({
@@ -93,7 +94,8 @@ describe("Zotero.Sync.Storage.Mode.ZFS", function () {
 				apiClient: client,
 				maxS3ConsecutiveFailures: 2
 			}),
-			stopOnError: true
+			background: options.background,
+			stopOnError
 		});
 		
 		return { engine, client, caller };
@@ -802,6 +804,75 @@ describe("Zotero.Sync.Storage.Mode.ZFS", function () {
 			var result = yield engine.start();
 			assert.equal(called, 4);
 		});
+		
+		
+		it("should stop uploading files on quota error", async function () {
+			var { engine, client, caller } = await setup({ stopOnError: false });
+			
+			var numItems = 4;
+			var items = [];
+			for (let i = 0; i < numItems; i++) {
+				let item = await importFileAttachment('test.png');
+				item.version = 5;
+				item.synced = true;
+				await item.saveTx();
+				items.push(item);
+			}
+			
+			var requests = 0;
+			server.respond(function (req) {
+				if (req.method == "POST"
+						&& req.url.startsWith(`${baseURL}users/1/items/`)
+						&& req.url.endsWith('/file')
+						&& req.requestBody.indexOf('upload=') == -1
+						&& req.requestHeaders["If-None-Match"] == "*") {
+					requests++;
+					req.respond(
+						413,
+						{
+							"Content-Type": "application/json",
+							"Last-Modified-Version": 10,
+							"Zotero-Storage-Usage": "300",
+							"Zotero-Storage-Quota": "300"
+						},
+						"File would exceed quota (299.7 + 0.5 > 300)"
+					);
+				}
+			})
+			
+			await engine.start();
+			assert.equal(requests, Zotero.Prefs.get('sync.storage.maxUploads'));
+			
+			Zotero.Sync.Storage.Local.storageRemainingForLibrary.delete(items[0].libraryID);
+		});
+		
+		
+		// If there was a quota error in a previous run and remaining storage was determined to be
+		// very low, stop further file uploads for a background sync even when we bail without an
+		// HTTP request. A manual sync clears the remaining-storage value.
+		it("should stop uploading files for background sync if no storage remaining after previous quota error", async function () {
+			var { engine, client, caller } = await setup({ background: true, stopOnError: false });
+			
+			var numItems = 4;
+			var items = [];
+			for (let i = 0; i < numItems; i++) {
+				let item = await importFileAttachment('test.png');
+				item.version = 5;
+				item.synced = true;
+				await item.saveTx();
+				items.push(item);
+			}
+			
+			Zotero.Sync.Storage.Local.storageRemainingForLibrary.set(items[0].libraryID, 0);
+			
+			var spy = sinon.spy(engine.controller, 'uploadFile');
+			await engine.start()
+			
+			assert.equal(spy.callCount, Zotero.Prefs.get('sync.storage.maxUploads'));
+			spy.restore();
+			
+			Zotero.Sync.Storage.Local.storageRemainingForLibrary.delete(items[0].libraryID);
+		});
 	})
 	
 	
@@ -830,7 +901,7 @@ describe("Zotero.Sync.Storage.Mode.ZFS", function () {
 			
 			var request = { name: item.libraryKey };
 			
-			var stub = sinon.stub(zfs, '_processUploadFile');
+			var stub = sinon.stub(zfs, '_processUploadFile').returns(Zotero.Promise.resolve());
 			await zfs.uploadFile(request);
 			
 			var zipFile = OS.Path.join(Zotero.getTempDirectory().path, item.key + '.zip');
@@ -1057,7 +1128,7 @@ describe("Zotero.Sync.Storage.Mode.ZFS", function () {
 							"Zotero-Storage-Usage": "300",
 							"Zotero-Storage-Quota": "300"
 						},
-						"File would exceed quota (299.7 + 0.5 &gt; 300)"
+						"File would exceed quota (299.7 + 0.5 > 300)"
 					);
 				}
 			})
@@ -1073,7 +1144,8 @@ describe("Zotero.Sync.Storage.Mode.ZFS", function () {
 			
 			// Try again
 			var e = yield getPromiseError(zfs.uploadFile({
-				name: item.libraryKey
+				name: item.libraryKey,
+				engine
 			}));
 			assert.ok(e);
 			assert.equal(e.errorType, 'warning');