Improve rebuilding of history database

- Re-add the force_rebuild key which we need internally again. This
partially reverts changes from:
  * cd0000f728459f208c4cf69f29b603fbcab6ffb4
  * 1a9b59fcfa73d1505834d8461ee166f07fb201cd
  * 93ecd8f72f108743948f0d1881055ff2337058ec

- Instead of checking self.completion to figure out whether we need to
  rebuild anything, check 'self' (i.e. the History table, not the
  CompletionHistory table). If something went wrong during the last
  rebuild, the CompletionHistory might still be empty, but History is what
  actually matters to figure out whether to rebuild.

- Set force_rebuild while rebuilding the history, so that a possible
  interruption of the process (e.g. by a killed process or crash)
  results in another rebuild from scratch.

- Bump up the user version again, so that we re-add force_rebuild to the
  database. This also forces another rebuild which helps with possible
  inconsistent data when someone interrupted the earlier rebuild for
  v2.0.0.

Fixes #6111

(cherry picked from commit 2b47bd01dbc15a02911989a190e2ef956aeedf27)

5 files changed, 51 insertions, 22 deletions

diff --git a/qutebrowser/browser/history.py b/qutebrowser/browser/history.py
index 8ce319cba..ef4650a35 100644
--- a/qutebrowser/browser/history.py
+++ b/qutebrowser/browser/history.py
@@ -88,6 +88,7 @@ class CompletionMetaInfo(sql.SqlTable):
 
     KEYS = {
         'excluded_patterns': '',
+        'force_rebuild': False,
     }
 
     def __init__(self, parent=None):
@@ -95,8 +96,6 @@ class CompletionMetaInfo(sql.SqlTable):
                          constraints={'key': 'PRIMARY KEY'})
         if sql.user_version_changed():
             self._init_default_values()
-            # force_rebuild is not in use anymore
-            self.delete('key', 'force_rebuild', optional=True)
 
     def _check_key(self, key):
         if key not in self.KEYS:
@@ -165,7 +164,7 @@ class WebHistory(sql.SqlTable):
         self.completion = CompletionHistory(parent=self)
         self.metainfo = CompletionMetaInfo(parent=self)
 
-        rebuild_completion = False
+        rebuild_completion = self.metainfo['force_rebuild']
 
         if sql.user_version_changed():
             # If the DB user version changed, run a full cleanup and rebuild the
@@ -186,8 +185,8 @@ class WebHistory(sql.SqlTable):
             self.metainfo['excluded_patterns'] = patterns
             rebuild_completion = True
 
-        if rebuild_completion and self.completion:
-            # If no completion history exists, we don't need to spawn a dialog for
+        if rebuild_completion and self:
+            # If no history exists, we don't need to spawn a dialog for
             # cleaning it up.
             self._rebuild_completion()
 
@@ -259,6 +258,10 @@ class WebHistory(sql.SqlTable):
         log.sql.debug(f"Cleanup removed {entries.rows_affected()} items")
 
     def _rebuild_completion(self):
+        # If this process was interrupted, make sure we trigger a rebuild again
+        # at the next start.
+        self.metainfo['force_rebuild'] = True
+
         data: Mapping[str, MutableSequence[str]] = {
             'url': [],
             'title': [],
@@ -305,6 +308,7 @@ class WebHistory(sql.SqlTable):
         QApplication.processEvents()
 
         self._progress.finish()
+        self.metainfo['force_rebuild'] = False
 
     def get_recent(self):
         """Get the most recent history entries."""
diff --git a/qutebrowser/misc/sql.py b/qutebrowser/misc/sql.py
index 774f3ebb0..7a3626f6e 100644
--- a/qutebrowser/misc/sql.py
+++ b/qutebrowser/misc/sql.py
@@ -66,7 +66,7 @@ class UserVersion:
 
 
 _db_user_version = None   # The user version we got from the database
-_USER_VERSION = UserVersion(0, 3)    # The current / newest user version
+_USER_VERSION = UserVersion(0, 4)    # The current / newest user version
 
 
 def user_version_changed():
@@ -409,13 +409,12 @@ class SqlTable(QObject):
         q.run()
         return q.query.next()
 
-    def delete(self, field, value, *, optional=False):
+    def delete(self, field, value):
         """Remove all rows for which `field` equals `value`.
 
         Args:
             field: Field to use as the key.
             value: Key value to delete.
-            optional: If set, non-existent values are ignored.
 
         Return:
             The number of rows deleted.
@@ -423,8 +422,6 @@ class SqlTable(QObject):
         q = Query(f"DELETE FROM {self._name} where {field} = :val")
         q.run(val=value)
         if not q.rows_affected():
-            if optional:
-                return
             raise KeyError('No row with {} = "{}"'.format(field, value))
         self.changed.emit()
 
diff --git a/tests/helpers/stubs.py b/tests/helpers/stubs.py
index c1ccdcf8b..de5cd981f 100644
--- a/tests/helpers/stubs.py
+++ b/tests/helpers/stubs.py
@@ -623,10 +623,11 @@ class FakeHistoryProgress:
 
     """Fake for a WebHistoryProgress object."""
 
-    def __init__(self):
+    def __init__(self, *, raise_on_tick=False):
         self._started = False
         self._finished = False
         self._value = 0
+        self._raise_on_tick = raise_on_tick
 
     def start(self, _text):
         self._started = True
@@ -635,6 +636,8 @@ class FakeHistoryProgress:
         pass
 
     def tick(self):
+        if self._raise_on_tick:
+            raise Exception('tick-tock')
         self._value += 1
 
     def finish(self):
diff --git a/tests/unit/browser/test_history.py b/tests/unit/browser/test_history.py
index 9e3bd0519..c032f6265 100644
--- a/tests/unit/browser/test_history.py
+++ b/tests/unit/browser/test_history.py
@@ -392,6 +392,25 @@ class TestRebuild:
             ('example.com/1', '', 1),
             ('example.com/2', '', 2),
         ]
+        assert not hist3.metainfo['force_rebuild']
+
+    def test_force_rebuild(self, web_history, stubs):
+        """Ensure that completion is regenerated if we force a rebuild."""
+        web_history.add_url(QUrl('example.com/1'), redirect=False, atime=1)
+        web_history.add_url(QUrl('example.com/2'), redirect=False, atime=2)
+        web_history.completion.delete('url', 'example.com/2')
+
+        hist2 = history.WebHistory(progress=stubs.FakeHistoryProgress())
+        # User version always changes, so this won't work
+        # assert list(hist2.completion) == [('example.com/1', '', 1)]
+        hist2.metainfo['force_rebuild'] = True
+
+        hist3 = history.WebHistory(progress=stubs.FakeHistoryProgress())
+        assert list(hist3.completion) == [
+            ('example.com/1', '', 1),
+            ('example.com/2', '', 2),
+        ]
+        assert not hist3.metainfo['force_rebuild']
 
     def test_exclude(self, config_stub, web_history, stubs):
         """Ensure that patterns in completion.web_history.exclude are ignored.
@@ -443,6 +462,23 @@ class TestRebuild:
         assert progress._started
         assert progress._finished
 
+    def test_interrupted(self, stubs, web_history, monkeypatch):
+        """If we interrupt the rebuilding process, force_rebuild should still be set."""
+        web_history.add_url(QUrl('example.com/1'), redirect=False, atime=1)
+        progress = stubs.FakeHistoryProgress(raise_on_tick=True)
+
+        # Trigger a completion rebuild
+        monkeypatch.setattr(sql, 'user_version_changed', lambda: True)
+
+        with pytest.raises(Exception, match='tick-tock'):
+            history.WebHistory(progress=progress)
+
+        assert web_history.metainfo['force_rebuild']
+
+        # If we now try again, we should get another rebuild. But due to user_version
+        # always changing, we can't test this at the moment (see the FIXME in the
+        # docstring for details)
+
 
 class TestCompletionMetaInfo:
 
@@ -466,12 +502,6 @@ class TestCompletionMetaInfo:
     def test_contains(self, metainfo):
         assert 'excluded_patterns' in metainfo
 
-    def test_delete_old_key(self, monkeypatch, metainfo):
-        metainfo.insert({'key': 'force_rebuild', 'value': False})
-        info2 = history.CompletionMetaInfo()
-        monkeypatch.setitem(info2.KEYS, 'force_rebuild', False)
-        assert 'force_rebuild' not in info2
-
     def test_modify(self, metainfo):
         assert not metainfo['excluded_patterns']
         value = 'https://example.com/'
diff --git a/tests/unit/misc/test_sql.py b/tests/unit/misc/test_sql.py
index 211280a6b..92bede511 100644
--- a/tests/unit/misc/test_sql.py
+++ b/tests/unit/misc/test_sql.py
@@ -227,11 +227,6 @@ def test_delete(qtbot):
     assert not list(table)
 
 
-def test_delete_optional(qtbot):
-    table = sql.SqlTable('Foo', ['name', 'val'])
-    table.delete('name', 'doesnotexist', optional=True)
-
-
 def test_len():
     table = sql.SqlTable('Foo', ['name', 'val', 'lucky'])
     assert len(table) == 0