Retry note saves that time out waiting for the database

Any error while saving a note prompted the user to restart Zotero, even
a transaction timeout caused by a long-running operation elsewhere.
Nothing has been written when the wait times out, so retry, unless newer
note content has been handed to the editor in the meantime.

https://forums.zotero.org/discussion/133298/

chrome/content/zotero/xpcom/editorInstance.js

@@ -50,10 +50,14 @@ const DOWNLOADED_IMAGE_TYPE = [
 	'image/png'
 ];
 
+// Tries at saving a note when the database is busy, each waiting out the transaction timeout
+const MAX_SAVE_ATTEMPTS = 3;
+
 class EditorInstance {
 	constructor() {
 		this.instanceID = Zotero.Utilities.randomString();
 		this._undoRedoController = null;
+		this._lastSaveID = 0;
 	}
 
 	get itemID() {
@@ -1140,7 +1144,9 @@ class EditorInstance {
 		}
 	}
 
-	async _save(noteData, skipDateModifiedUpdate) {
+	// saveID identifies this save among the editor's others, so that a retry can tell whether it
+	// still holds the newest content. Assigned below, once there's something to write.
+	async _save(noteData, skipDateModifiedUpdate, saveID, attempt = 1) {
 		if (!noteData) return;
 		let { state, html } = noteData;
 		if (html === undefined) return;
@@ -1158,6 +1164,13 @@ class EditorInstance {
 				Zotero.debug('Note value not available -- not saving', 2);
 				return;
 			}
+			// Claim an ID now that there's something to write. saveSync() calls through with no
+			// data whenever the editor has no unsaved changes -- including once an update has been
+			// dispatched but not yet saved -- and such a save must not make a retry waiting below
+			// look superseded.
+			if (saveID === undefined) {
+				saveID = ++this._lastSaveID;
+			}
 			// Update note
 			if (this._item) {
 				await Zotero.DB.executeTransaction(async () => {
@@ -1199,6 +1212,20 @@ class EditorInstance {
 			}
 		}
 		catch (e) {
+			// A long-running operation elsewhere (e.g., full-text index maintenance) can hold the
+			// database past the transaction wait timeout. Nothing was written, so try again rather
+			// than telling the user to restart.
+			if (e instanceof Zotero.DBConnection.TimeoutError && attempt < MAX_SAVE_ATTEMPTS) {
+				// Unless the editor has handed us newer content in the meantime, in which case
+				// this save is superseded and retrying it would undo the newer one
+				if (saveID != this._lastSaveID) {
+					Zotero.debug("Timed out saving note, but a newer save is pending -- skipping", 2);
+					return;
+				}
+				Zotero.debug("Timed out waiting for the database to save note -- retrying", 2);
+				await this._save(noteData, skipDateModifiedUpdate, saveID, attempt + 1);
+				return;
+			}
 			Zotero.logError(e);
 			Zotero.crash(true);
 			throw e;

test/tests/noteeditorTest.js

@@ -175,4 +175,60 @@ describe("Note Editor", function () {
 			);
 		});
 	});
+
+	describe("Saving", function () {
+		it("should retry a save that times out waiting for the database", async function () {
+			let editorInstance = await openEditor();
+			let crash = sinon.stub(Zotero, 'crash');
+			let executeTransaction = sinon.stub(Zotero.DB, 'executeTransaction');
+			executeTransaction.onFirstCall().rejects(new Zotero.DBConnection.TimeoutError());
+			executeTransaction.callThrough();
+			try {
+				await editorInstance._save({ state: {}, html: '<p>Retried note</p>' });
+			}
+			finally {
+				executeTransaction.restore();
+				crash.restore();
+			}
+			assert.isTrue(crash.notCalled);
+			assert.equal(editorInstance._item.getNote(), '<p>Retried note</p>');
+		});
+
+		it("shouldn't let a retried save overwrite newer note content", async function () {
+			let editorInstance = await openEditor();
+			let crash = sinon.stub(Zotero, 'crash');
+			let executeTransaction = sinon.stub(Zotero.DB, 'executeTransaction');
+			executeTransaction.onFirstCall().rejects(new Zotero.DBConnection.TimeoutError());
+			executeTransaction.callThrough();
+			try {
+				let timedOut = editorInstance._save({ state: {}, html: '<p>Older note</p>' });
+				await editorInstance._save({ state: {}, html: '<p>Newer note</p>' });
+				await timedOut;
+			}
+			finally {
+				executeTransaction.restore();
+				crash.restore();
+			}
+			assert.equal(editorInstance._item.getNote(), '<p>Newer note</p>');
+		});
+
+		it("shouldn't let a save with nothing to save supersede a pending retry", async function () {
+			let editorInstance = await openEditor();
+			let crash = sinon.stub(Zotero, 'crash');
+			let executeTransaction = sinon.stub(Zotero.DB, 'executeTransaction');
+			executeTransaction.onFirstCall().rejects(new Zotero.DBConnection.TimeoutError());
+			executeTransaction.callThrough();
+			try {
+				let timedOut = editorInstance._save({ state: {}, html: '<p>Pending note</p>' });
+				// What saveSync() passes through when the editor has no unsaved changes
+				await editorInstance._save(null);
+				await timedOut;
+			}
+			finally {
+				executeTransaction.restore();
+				crash.restore();
+			}
+			assert.equal(editorInstance._item.getNote(), '<p>Pending note</p>');
+		});
+	});
 });