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
@@ -395,3 +404,47 @@ 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(),
+        ])