commit immediately when there's no active checkpoint

Reviews and operations on the backend that support undoing can now be
committed immediately, so they will not be lost in the event of a crash.

This required tweaks to a few places:

- don't set collection mtime on save() unless changes were made in
Python, as otherwise we end up accidentally clearing the backend undo
queue
- autosave() is now run on every reset()
- garbage collection now runs in a timer, instead of relying on
autosave() to be run periodically

pylib/anki/collection.py

@@ -90,9 +90,6 @@ class Collection:
         self.path = os.path.abspath(path)
         self.reopen()
 
-        self.log(self.path, anki.version)
-        self._lastSave = time.time()
-        self._undo: _UndoInfo = None
         self.media = MediaManager(self, server)
         self.models = ModelManager(self)
         self.decks = DeckManager(self)
@@ -230,15 +227,18 @@ class Collection:
         # to check if the backend updated the modification time.
         return self.db.last_begin_at != self.mod
 
-    def save(
+    def save(self, name: Optional[str] = None, trx: bool = True) -> None:
-        self, name: Optional[str] = None, mod: Optional[int] = None, trx: bool = True
-    ) -> None:
         "Flush, commit DB, and take out another write lock if trx=True."
         # commit needed?
-        if self.db.mod or self.modified_after_begin():
+        if self.db.modified_in_python or self.modified_after_begin():
-            self.mod = intTime(1000) if mod is None else mod
+            if self.db.modified_in_python:
+                self.db.execute("update col set mod = ?", intTime(1000))
+                self.db.modified_in_python = False
+            else:
+                # modifications made by the backend will have already bumped
+                # mtime
+                pass
             self.db.commit()
-            self.db.mod = False
             if trx:
                 self.db.begin()
         elif not trx:
@@ -247,14 +247,15 @@ class Collection:
             self.db.rollback()
 
         self._save_checkpoint(name)
-        self._lastSave = time.time()
 
-    def autosave(self) -> Optional[bool]:
+    def autosave(self) -> None:
-        "Save if 5 minutes has passed since last save. True if saved."
+        """Save any pending changes.
-        if time.time() - self._lastSave > 300:
+        If a checkpoint was taken in the last 5 minutes, don't save."""
+        if not self._have_outstanding_checkpoint():
+            # if there's no active checkpoint, we can save immediately
+            self.save()
+        elif time.time() - self._last_checkpoint_at > 300:
             self.save()
-            return True
-        return None
 
     def close(self, save: bool = True, downgrade: bool = False) -> None:
         "Disconnect from DB."
@@ -290,6 +291,9 @@ class Collection:
         assert not self.db
         assert self.path.endswith(".anki2")
 
+        self._last_checkpoint_at = time.time()
+        self._undo: _UndoInfo = None
+
         (media_dir, media_db) = media_paths_from_col_path(self.path)
 
         log_path = ""
@@ -355,6 +359,7 @@ class Collection:
 
     def reset(self) -> None:
         "Rebuild the queue and reload data after DB modified."
+        self.autosave()
         self.sched.reset()
 
     # Deletion logging
@@ -752,17 +757,16 @@ table.review-log {{ {revlog_style} }}
     def undo_status(self) -> UndoStatus:
         "Return the undo status. At the moment, redo is not supported."
         # check backend first
-        status = self._backend.get_undo_status()
+        if status := self._check_backend_undo_status():
-        if status.undo or status.redo:
             return status
 
         if not self._undo:
-            return status
+            return UndoStatus()
 
         if isinstance(self._undo, _ReviewsUndo):
-            status.undo = self.tr(TR.SCHEDULING_REVIEW)
+            return UndoStatus(undo=self.tr(TR.SCHEDULING_REVIEW))
         elif isinstance(self._undo, Checkpoint):
-            status.undo = self._undo.name
+            return UndoStatus(undo=self._undo.name)
         else:
             assert_exhaustive(self._undo)
             assert False
@@ -796,6 +800,16 @@ table.review-log {{ {revlog_style} }}
             assert_exhaustive(self._undo)
             assert False
 
+    def _check_backend_undo_status(self) -> Optional[UndoStatus]:
+        """Return undo status if undo available on backend.
+        If backend has undo available, clear the Python undo state."""
+        status = self._backend.get_undo_status()
+        if status.undo or status.redo:
+            self.clear_python_undo()
+            return status
+        else:
+            return None
+
     def save_card_review_undo_info(self, card: Card) -> None:
         "Used by V1 and V2 schedulers to record state prior to review."
         if not isinstance(self._undo, _ReviewsUndo):
@@ -805,6 +819,10 @@ table.review-log {{ {revlog_style} }}
         entry = ReviewUndo(card=copy.copy(card), was_leech=was_leech)
         self._undo.entries.append(entry)
 
+    def _have_outstanding_checkpoint(self) -> bool:
+        self._check_backend_undo_status()
+        return isinstance(self._undo, Checkpoint)
+
     def _undo_checkpoint(self) -> Checkpoint:
         assert isinstance(self._undo, Checkpoint)
         self.rollback()
@@ -814,6 +832,7 @@ table.review-log {{ {revlog_style} }}
 
     def _save_checkpoint(self, name: Optional[str]) -> None:
         "Call via .save(). If name not provided, clear any existing checkpoint."
+        self._last_checkpoint_at = time.time()
         if name:
             self._undo = Checkpoint(name=name)
         else:

pylib/anki/dbproxy.py

@@ -24,7 +24,7 @@ class DBProxy:
 
     def __init__(self, backend: anki._backend.RustBackend) -> None:
         self._backend = backend
-        self.mod = False
+        self.modified_in_python = False
         self.last_begin_at = 0
 
     # Transactions
@@ -54,7 +54,7 @@ class DBProxy:
         s = sql.strip().lower()
         for stmt in "insert", "update", "delete":
             if s.startswith(stmt):
-                self.mod = True
+                self.modified_in_python = True
         sql, args2 = emulate_named_args(sql, args, kwargs)
         # fetch rows
         return self._backend.db_query(sql, args2, first_row_only)
@@ -92,7 +92,7 @@ class DBProxy:
     ################
 
     def executemany(self, sql: str, args: Iterable[Sequence[ValueForDB]]) -> None:
-        self.mod = True
+        self.modified_in_python = True
         if isinstance(args, list):
             list_args = args
         else:

pylib/tests/test_undo.py

@@ -22,7 +22,7 @@ def test_op():
     # it should be listed as undoable
     assert col.undoName() == "studyopts"
     # with about 5 minutes until it's clobbered
-    assert time.time() - col._lastSave < 1
+    assert time.time() - col._last_checkpoint_at < 1
     # undoing should restore the old value
     col.undo()
     assert not col.undoName()

qt/aqt/about.py

@@ -28,7 +28,7 @@ class ClosableQDialog(QDialog):
 def show(mw: aqt.AnkiQt) -> QDialog:
     dialog = ClosableQDialog(mw)
     disable_help_button(dialog)
-    mw.setupDialogGC(dialog)
+    mw.garbage_collect_on_dialog_finish(dialog)
     abt = aqt.forms.about.Ui_About()
     abt.setupUi(dialog)
 

qt/aqt/addcards.py

@@ -34,7 +34,7 @@ from aqt.utils import (
 class AddCards(QDialog):
     def __init__(self, mw: AnkiQt) -> None:
         QDialog.__init__(self, None, Qt.Window)
-        mw.setupDialogGC(self)
+        mw.garbage_collect_on_dialog_finish(self)
         self.mw = mw
         self.form = aqt.forms.addcards.Ui_Dialog()
         self.form.setupUi(self)

qt/aqt/browser.py

@@ -575,7 +575,7 @@ class Browser(QMainWindow):
         self.mw.maybeReset()
         aqt.dialogs.markClosed("Browser")
         self._closeEventHasCleanedUp = True
-        self.mw.gcWindow(self)
+        self.mw.deferred_delete_and_garbage_collect(self)
         self.close()
 
     def closeWithCallback(self, onsuccess: Callable) -> None:
@@ -1556,7 +1556,7 @@ where id in %s"""
 
     def _onFindDupes(self) -> None:
         d = QDialog(self)
-        self.mw.setupDialogGC(d)
+        self.mw.garbage_collect_on_dialog_finish(d)
         frm = aqt.forms.finddupes.Ui_Dialog()
         frm.setupUi(d)
         restoreGeom(d, "findDupes")

qt/aqt/clayout.py

@@ -47,7 +47,7 @@ class CardLayout(QDialog):
         fill_empty: bool = False,
     ) -> None:
         QDialog.__init__(self, parent or mw, Qt.Window)
-        mw.setupDialogGC(self)
+        mw.garbage_collect_on_dialog_finish(self)
         self.mw = aqt.mw
         self.note = note
         self.ord = ord

qt/aqt/editcurrent.py

@@ -10,7 +10,7 @@ from aqt.utils import TR, disable_help_button, restoreGeom, saveGeom, tooltip, t
 class EditCurrent(QDialog):
     def __init__(self, mw: aqt.AnkiQt) -> None:
         QDialog.__init__(self, None, Qt.Window)
-        mw.setupDialogGC(self)
+        mw.garbage_collect_on_dialog_finish(self)
         self.mw = mw
         self.form = aqt.forms.editcurrent.Ui_Dialog()
         self.form.setupUi(self)

qt/aqt/main.py

@@ -148,7 +148,7 @@ class AnkiQt(QMainWindow):
     def setupUI(self) -> None:
         self.col = None
         self.setupCrashLog()
-        self.disableGC()
+        self.disable_automatic_garbage_collection()
         self.setupAppMsg()
         self.setupKeys()
         self.setupThreads()
@@ -1090,10 +1090,8 @@ title="%s" %s>%s</button>""" % (
         self.maybeEnableUndo()
 
     def autosave(self) -> None:
-        saved = self.col.autosave()
+        self.col.autosave()
         self.maybeEnableUndo()
-        if saved:
-            self.doGC()
 
     # Other menu operations
     ##########################################################################
@@ -1256,6 +1254,8 @@ title="%s" %s>%s</button>""" % (
         self.progress.timer(10 * 60 * 1000, self.onRefreshTimer, True)
         # check media sync every 5 minutes
         self.progress.timer(5 * 60 * 1000, self.on_autosync_timer, True)
+        # periodic garbage collection
+        self.progress.timer(15 * 60 * 1000, self.garbage_collect_now, False)
         # ensure Python interpreter runs at least once per second, so that
         # SIGINT/SIGTERM is processed without a long delay
         self.progress.timer(1000, lambda: None, True, False)
@@ -1621,22 +1621,39 @@ title="%s" %s>%s</button>""" % (
 
     # GC
     ##########################################################################
-    # ensure gc runs in main thread
+    # The default Python garbage collection can trigger on any thread. This can
+    # cause crashes if Qt objects are garbage-collected, as Qt expects access
+    # only on the main thread. So Anki disables the default GC on startup, and
+    # instead runs it on a timer, and after dialog close.
+    # The gc after dialog close is necessary to free up the memory and extra
+    # processes that webviews spawn, as a lot of the GUI code creates ref cycles.
 
-    def setupDialogGC(self, obj: Any) -> None:
+    def garbage_collect_on_dialog_finish(self, dialog: QDialog) -> None:
-        qconnect(obj.finished, lambda: self.gcWindow(obj))
+        qconnect(
+            dialog.finished, lambda: self.deferred_delete_and_garbage_collect(dialog)
+        )
 
-    def gcWindow(self, obj: Any) -> None:
+    def deferred_delete_and_garbage_collect(self, obj: QObject) -> None:
         obj.deleteLater()
-        self.progress.timer(1000, self.doGC, False, requiresCollection=False)
+        self.progress.timer(
+            1000, self.garbage_collect_now, False, requiresCollection=False
+        )
 
-    def disableGC(self) -> None:
+    def disable_automatic_garbage_collection(self) -> None:
         gc.collect()
         gc.disable()
 
-    def doGC(self) -> None:
+    def garbage_collect_now(self) -> None:
+        # gc.collect() has optional arguments that will cause problems if
+        # it's passed directly to a QTimer, and pylint complains if we
+        # wrap it in a lambda, so we use this trivial wrapper
         gc.collect()
 
+    # legacy aliases
+
+    setupDialogGC = garbage_collect_on_dialog_finish
+    gcWindow = deferred_delete_and_garbage_collect
+
     # Crash log
     ##########################################################################
 

qt/aqt/stats.py

@@ -27,7 +27,7 @@ class NewDeckStats(QDialog):
 
     def __init__(self, mw: aqt.main.AnkiQt) -> None:
         QDialog.__init__(self, mw, Qt.Window)
-        mw.setupDialogGC(self)
+        mw.garbage_collect_on_dialog_finish(self)
         self.mw = mw
         self.name = "deckStats"
         self.period = 0
@@ -108,7 +108,7 @@ class DeckStats(QDialog):
 
     def __init__(self, mw: aqt.main.AnkiQt) -> None:
         QDialog.__init__(self, mw, Qt.Window)
-        mw.setupDialogGC(self)
+        mw.garbage_collect_on_dialog_finish(self)
         self.mw = mw
         self.name = "deckStats"
         self.period = 0