Fix ordering of complete_session: (this has a new bug that we set the…

… config_keys before completing the session.. if any error raises we have updated config keys, but not updated files in remote settings which could be a critical bug

src/olympia/blocklist/tasks.py

@@ -145,28 +145,6 @@ def upload_filter(generation_time, filter_list=None, create_stash=False):
             server.publish_record(stash_upload_data)
             statsd.incr('blocklist.tasks.upload_filter.upload_stash')
 
-    # Commit the changes to remote settings for review.
-    # only after this can we safely update config timestamps
-    server.complete_session()
-    set_config(MLBF_TIME_CONFIG_KEY, generation_time, json_value=True)
-
-    # Update the base_filter_id for uploaded filters
-    for block_type in filter_list:
-        # We currently write to the old singular config key for hard blocks
-        # to preserve backward compatibility.
-        # In https://github.com/mozilla/addons/issues/15193
-        # we can remove this and start writing to the new plural key.
-        if block_type == BlockType.BLOCKED:
-            set_config(
-                MLBF_BASE_ID_CONFIG_KEY(block_type, compat=True),
-                generation_time,
-                json_value=True,
-            )
-
-        set_config(
-            MLBF_BASE_ID_CONFIG_KEY(block_type), generation_time, json_value=True
-        )
-
     oldest_base_filter_id: int | None = None
 
     # Get the oldest base_filter_id from the set of defined IDs
@@ -206,6 +184,30 @@ def upload_filter(generation_time, filter_list=None, create_stash=False):
             if record_time < oldest_base_filter_id:
                 server.delete_record(record['id'])
 
+    # Commit the changes to remote settings for review.
+    # only after any changes to records (attachments and stashes)
+    # and including deletions can we commit the session
+    # and update the config with the new timestamps
+    server.complete_session()
+    set_config(MLBF_TIME_CONFIG_KEY, generation_time, json_value=True)
+
+    # Update the base_filter_id for uploaded filters
+    for block_type in filter_list:
+        # We currently write to the old singular config key for hard blocks
+        # to preserve backward compatibility.
+        # In https://github.com/mozilla/addons/issues/15193
+        # we can remove this and start writing to the new plural key.
+        if block_type == BlockType.BLOCKED:
+            set_config(
+                MLBF_BASE_ID_CONFIG_KEY(block_type, compat=True),
+                generation_time,
+                json_value=True,
+            )
+
+        set_config(
+            MLBF_BASE_ID_CONFIG_KEY(block_type), generation_time, json_value=True
+        )
+
     cleanup_old_files.delay(base_filter_id=oldest_base_filter_id)
     statsd.incr('blocklist.tasks.upload_filter.reset_collection')
 

src/olympia/blocklist/tests/test_tasks.py

@@ -256,13 +256,21 @@ def test_upload_soft_and_blocked_filter(self):
     def _test_cleanup_old_records(
         self,
         filter_list: Dict[BlockType, int],
+        create_stash: bool,
         records: List[Dict[str, int]],
         expected_calls: List[any],
     ):
         self._block_version(is_signed=True)
         mlbf = MLBF.generate_from_db(self.generation_time)
+
+        create_filter = len(filter_list) > 0
+
+        if create_stash:
+            mlbf.generate_and_write_stash()
+
         for block_type, base_id in filter_list.items():
-            mlbf.generate_and_write_filter(block_type)
+            if create_filter:
+                mlbf.generate_and_write_filter(block_type)
             # We currently write to the old singular config key for hard blocks
             # to preserve backward compatibility.
             # In https://github.com/mozilla/addons/issues/15193
@@ -277,6 +285,7 @@ def _test_cleanup_old_records(
         upload_filter.delay(
             self.generation_time,
             filter_list=[block_type.name for block_type in filter_list],
+            create_stash=create_stash,
         )
 
         assert self.mocks['delete_record'].call_args_list == expected_calls
@@ -294,6 +303,7 @@ def test_skip_cleanup_when_no_filters(self):
             filter_list={},
             records=[{'id': '0', 'generation_time': self.generation_time}],
             expected_calls=[],
+            create_stash=False,
         )
 
     def test_cleanup_old_records(self):
@@ -313,6 +323,7 @@ def test_cleanup_old_records(self):
                 self._stash(2, self.generation_time + 1),
             ],
             expected_calls=[mock.call(0)],
+            create_stash=False,
         )
 
     def test_cleanup_oldest_stash_records(self):
@@ -340,6 +351,7 @@ def test_cleanup_oldest_stash_records(self):
                 self._stash(2, self.generation_time - 1),
             ],
             expected_calls=[mock.call(0), mock.call(1)],
+            create_stash=False,
         )
 
     def test_create_stashed_filter(self):
@@ -395,3 +407,79 @@ def test_default_is_no_op(self):
         upload_filter.delay(self.generation_time)
         assert not self.mocks['delete_record'].called
         assert not self.mocks['publish_record'].called
+
+    def test_complete_session_is_called_after_upload_stash(self):
+        """
+        server.complete_session is called to "commit" all of the changes made
+        to remote settings. For this reason, it is important that we call it
+        after uploading the new stash.
+        """
+        mlbf = MLBF.generate_from_db(self.generation_time)
+        mlbf.generate_and_write_stash()
+        mlbf.generate_and_write_filter(BlockType.BLOCKED)
+
+        mocks = [
+            'delete_record',
+            'complete_session',
+            'records',
+            'publish_record',
+            'publish_attachment',
+        ]
+        manager = mock.Mock()
+
+        for mock_name in mocks:
+            mock_patch = self.mocks[mock_name]
+            manager.attach_mock(mock_patch, mock_name)
+
+        attachment_record = self._attachment(
+            0, 'bloomfilter-base', self.generation_time - 1
+        )
+        stash_record = self._stash(1, self.generation_time - 2)
+
+        manager.records.return_value = [attachment_record, stash_record]
+
+        with mock.patch('olympia.blocklist.tasks.RemoteSettings', return_value=manager):
+            self._test_cleanup_old_records(
+                filter_list={BlockType.BLOCKED: self.generation_time - 1},
+                records=[attachment_record, stash_record],
+                # We expect to delete the attachment and the stash record
+                expected_calls=[mock.call(0), mock.call(1)],
+                create_stash=True,
+            )
+
+        manager.assert_has_calls(
+            [
+                # First we get the existing records
+                mock.call.records(),
+                # Then we publish the new attachment (and stash)
+                mock.call.publish_attachment(mock.ANY, mock.ANY),
+                mock.call.publish_record(mock.ANY),
+                # Then we delete the old attachment and stash
+                mock.call.delete_record(attachment_record['id']),
+                mock.call.delete_record(stash_record['id']),
+                # Then we commit the changes
+                mock.call.complete_session(),
+            ]
+        )
+
+    def test_config_is_updated_only_after_complete_session(self):
+        """
+        We should not update the config until after we've committed the session
+        Since this call to complete session will raise an exception, the config
+        should not be updated.
+        """
+        mlbf = MLBF.generate_from_db(self.generation_time)
+        mlbf.generate_and_write_stash()
+        mlbf.generate_and_write_filter(BlockType.BLOCKED)
+
+        self.mocks['complete_session'].side_effect = Exception('Something went wrong')
+
+        with self.assertRaises(Exception):  # noqa: B017
+            upload_filter.delay(
+                self.generation_time,
+                filter_list=[BlockType.BLOCKED.name],
+                create_stash=True,
+            )
+
+        assert self.mocks['complete_session'].called
+        assert not self.mocks['set_config'].called