Ensure that migration of files is commited before deleting files

When moving attachments from the filestore to an object storage,  the
filesystem files will be deleted only after the commit, so if the
transaction is rollbacked, we still have the local files for another
try.

base_attachment_object_storage/models/ir_attachment.py

@@ -16,6 +16,25 @@ from odoo import api, exceptions, models, _
 _logger = logging.getLogger(__name__)
 
 
+def clean_fs(files):
+    _logger.info('cleaning old files from filestore')
+    for full_path in files:
+        if os.path.exists(full_path):
+            try:
+                os.unlink(full_path)
+            except OSError:
+                _logger.info(
+                    "_file_delete could not unlink %s",
+                    full_path, exc_info=True
+                )
+            except IOError:
+                # Harmless and needed for race conditions
+                _logger.info(
+                    "_file_delete could not unlink %s",
+                    full_path, exc_info=True
+                )
+
+
 class IrAttachment(models.Model):
     _inherit = 'ir.attachment'
 
@@ -178,22 +197,7 @@ class IrAttachment(models.Model):
                         # on assets
                         'mimetype': self.mimetype})
             _logger.info('moved %s on the object storage', fname)
-            full_path = self._full_path(fname)
-            _logger.info('cleaning fs self')
-            if os.path.exists(full_path):
-                try:
-                    os.unlink(full_path)
-                except OSError:
-                    _logger.info(
-                        "_file_delete could not unlink %s",
-                        full_path, exc_info=True
-                    )
-                except IOError:
-                    # Harmless and needed for race conditions
-                    _logger.info(
-                        "_file_delete could not unlink %s",
-                        full_path, exc_info=True
-                    )
+            return self._full_path(fname)
         elif self.db_datas:
             _logger.info('moving on the object storage from database')
             self.write({'datas': self.datas})
@@ -223,11 +227,12 @@ class IrAttachment(models.Model):
         with self.do_in_new_env(new_cr=new_cr) as new_env:
             model_env = new_env['ir.attachment']
             ids = model_env.search(domain).ids
+            files_to_clean = []
             for attachment_id in ids:
                 try:
                     with new_env.cr.savepoint():
                         # check that no other transaction has
-                        # locked the row, don't send a file to S3
+                        # locked the row, don't send a file to storage
                         # in that case
                         self.env.cr.execute("SELECT id "
                                             "FROM ir_attachment "
@@ -243,11 +248,21 @@ class IrAttachment(models.Model):
                         # ALL the attachments on each loop.
                         new_env.clear()
                         attachment = model_env.browse(attachment_id)
-                        attachment._move_attachment_to_store()
+                        path = attachment._move_attachment_to_store()
+                        if path:
+                            files_to_clean.append(path)
                 except psycopg2.OperationalError:
                     _logger.error('Could not migrate attachment %s to S3',
                                   attachment_id)
 
+            def clean():
+                clean_fs(files_to_clean)
+
+            # delete the files from the filesystem once we know the changes
+            # have been committed in ir.attachment
+            if files_to_clean:
+                new_env.cr.after('commit', clean)
+
     def _get_stores(self):
         """ To get the list of stores activated in the system  """
         return []