stop disabling django autocommit

Author: matt-codecov

django_scaffold/settings.py

@@ -8,10 +8,6 @@
 
 ALLOWED_HOSTS = []
 
-DATABASES["default"]["AUTOCOMMIT"] = False
-if "timeseries" in DATABASES:
-    DATABASES["timeseries"]["AUTOCOMMIT"] = False
-
 IS_DEV = os.getenv("RUN_ENV") == "DEV"
 IS_ENTERPRISE = os.getenv("RUN_ENV") == "ENTERPRISE"
 

services/cleanup/tests/test_utils.py

@@ -3,7 +3,6 @@
 from contextlib import contextmanager
 
 import shared.storage
-from django.db import transaction
 from django.db.models import Model
 from shared.config import get_config
 from shared.storage.base import BaseStorageService
@@ -35,25 +34,6 @@ def cleanup_context():
         context.threadpool.shutdown()
 
 
-@contextmanager
-def with_autocommit():
-    conn = transaction.get_connection()
-    # if we are in an `atomic` block, we cannot mess with the autocommit flag
-    if conn.in_atomic_block:
-        yield
-        return
-
-    try:
-        autocommit = conn.get_autocommit()
-        if autocommit != True:
-            # we first have to commit an implicitly running transaction
-            conn.commit()
-            conn.set_autocommit(True)
-        yield
-    finally:
-        conn.set_autocommit(autocommit)
-
-
 @dataclasses.dataclass
 class CleanupResult:
     cleaned_models: int

services/cleanup/utils.py

@@ -5,10 +5,8 @@
 from celery._state import get_current_task
 from celery.exceptions import MaxRetriesExceededError, SoftTimeLimitExceeded
 from celery.worker.request import Request
-from django.db import transaction as django_transaction
 from shared.celery_router import route_tasks_based_on_user_plan
 from shared.metrics import Counter, Histogram
-from shared.timeseries.helpers import is_timeseries_enabled
 from shared.torngit.base import TorngitBaseAdapter
 from shared.typings.torngit import AdditionalData
 from sqlalchemy.exc import (
@@ -180,46 +178,6 @@ def apply_async(self, args=None, kwargs=None, **options):
         }
         return super().apply_async(args=args, kwargs=kwargs, headers=headers, **options)
 
-    def _commit_django(self):
-        try:
-            django_transaction.commit()
-        except Exception as e:
-            log.warning(
-                "Django transaction failed to commit.",
-                exc_info=True,
-                extra=dict(e=e),
-            )
-
-        if is_timeseries_enabled():
-            try:
-                django_transaction.commit("timeseries")
-            except Exception as e:
-                log.warning(
-                    "Django transaction failed to commit in the timeseries database.",
-                    exc_info=True,
-                    extra=dict(e=e),
-                )
-
-    def _rollback_django(self):
-        try:
-            django_transaction.rollback()
-        except Exception as e:
-            log.warning(
-                "Django transaction failed to roll back.",
-                exc_info=True,
-                extra=dict(e=e),
-            )
-
-        if is_timeseries_enabled():
-            try:
-                django_transaction.rollback("timeseries")
-            except Exception as e:
-                log.warning(
-                    "Django transaction failed to roll back in the timeseries database.",
-                    exc_info=True,
-                    extra=dict(e=e),
-                )
-
     # Called when attempting to retry the task on db error
     def _retry(self, countdown=None):
         if not countdown:
@@ -307,12 +265,10 @@ def run(self, *args, **kwargs):
                     extra=dict(task_args=args, task_kwargs=kwargs),
                 )
                 db_session.rollback()
-                self._rollback_django()
                 self._retry()
             except SQLAlchemyError as ex:
                 self._analyse_error(ex, args, kwargs)
                 db_session.rollback()
-                self._rollback_django()
                 self._retry()
             except MaxRetriesExceededError as ex:
                 if UploadFlow.has_begun():
@@ -321,7 +277,6 @@ def run(self, *args, **kwargs):
                     TestResultsFlow.log(TestResultsFlow.UNCAUGHT_RETRY_EXCEPTION)
             finally:
                 self.wrap_up_dbsession(db_session)
-                self._commit_django()
 
     def wrap_up_dbsession(self, db_session):
         """

tasks/base.py

@@ -5,7 +5,7 @@
 
 from app import celery_app
 from services.cleanup.owner import cleanup_owner
-from services.cleanup.utils import CleanupSummary, with_autocommit
+from services.cleanup.utils import CleanupSummary
 from tasks.base import BaseCodecovTask
 
 log = logging.getLogger(__name__)
@@ -17,8 +17,7 @@ class DeleteOwnerTask(BaseCodecovTask, name=delete_owner_task_name):
 
     def run_impl(self, _db_session, ownerid: int) -> CleanupSummary:
         try:
-            with with_autocommit():
-                return cleanup_owner(ownerid)
+            return cleanup_owner(ownerid)
         except SoftTimeLimitExceeded:
             raise self.retry()
 

tasks/delete_owner.py

@@ -4,7 +4,7 @@
 
 from app import celery_app
 from services.cleanup.repository import cleanup_repo
-from services.cleanup.utils import CleanupSummary, with_autocommit
+from services.cleanup.utils import CleanupSummary
 from tasks.base import BaseCodecovTask
 
 log = logging.getLogger(__name__)
@@ -16,8 +16,7 @@ class FlushRepoTask(BaseCodecovTask, name="app.tasks.flush_repo.FlushRepo"):
 
     def run_impl(self, _db_session, repoid: int) -> CleanupSummary:
         try:
-            with with_autocommit():
-                return cleanup_repo(repoid)
+            return cleanup_repo(repoid)
         except SoftTimeLimitExceeded:
             raise self.retry()
 

tasks/flush_repo.py

@@ -5,7 +5,7 @@
 from app import celery_app
 from celery_config import regular_cleanup_cron_task_name
 from services.cleanup.regular import run_regular_cleanup
-from services.cleanup.utils import CleanupSummary, with_autocommit
+from services.cleanup.utils import CleanupSummary
 from tasks.base import BaseCodecovTask
 
 log = logging.getLogger(__name__)
@@ -17,8 +17,7 @@ class RegularCleanupTask(BaseCodecovTask, name=regular_cleanup_cron_task_name):
 
     def run_impl(self, _db_session, *args, **kwargs) -> CleanupSummary:
         try:
-            with with_autocommit():
-                return run_regular_cleanup()
+            return run_regular_cleanup()
         except SoftTimeLimitExceeded:
             raise self.retry()
 

tasks/regular_cleanup.py

@@ -133,7 +133,6 @@ def setup_mocks(
     hook_session(mocker, dbsession)
     # to not close the session after each task
     mocker.patch("tasks.base.BaseCodecovTask.wrap_up_dbsession")
-    mocker.patch("tasks.base.BaseCodecovTask._commit_django")
     # patch various `get_repo_provider_service` imports
     hook_repo_provider(mocker, mock_repo_provider)
     # avoid some calls reaching out to git providers

tasks/tests/integration/test_upload_e2e.py

@@ -11,7 +11,6 @@
 from prometheus_client import REGISTRY
 from shared.celery_config import sync_repos_task_name, upload_task_name
 from shared.plan.constants import PlanName
-from shared.utils.test_utils import mock_config_helper
 from sqlalchemy.exc import (
     DBAPIError,
     IntegrityError,
@@ -208,40 +207,8 @@ def test_wrap_up_dbsession_invalid_nothing_works(self, mocker):
         assert fake_session.close.call_count == 0
         assert mocked_get_db_session.remove.call_count == 1
 
-    def test_commit_django_with_timeseries(self, mocker):
-        mock_config_helper(mocker, configs={"setup.timeseries.enabled": True})
-        mock_commit = mocker.patch("tasks.base.django_transaction.commit")
-        task = BaseCodecovTask()
-        task._commit_django()
-        assert mock_commit.call_args_list == [call(), call("timeseries")]
-
-    def test_commit_django_without_timeseries(self, mocker):
-        mock_config_helper(mocker, configs={"setup.timeseries.enabled": False})
-        mock_commit = mocker.patch("tasks.base.django_transaction.commit")
-        task = BaseCodecovTask()
-        task._commit_django()
-        assert mock_commit.call_args_list == [call()]
-
-    def test_rollback_django_with_timeseries(self, mocker):
-        mock_config_helper(mocker, configs={"setup.timeseries.enabled": True})
-        mock_rollback = mocker.patch("tasks.base.django_transaction.rollback")
-        task = BaseCodecovTask()
-        task._rollback_django()
-        assert mock_rollback.call_args_list == [call(), call("timeseries")]
-
-    def test_rollback_django_without_timeseries(self, mocker):
-        mock_config_helper(mocker, configs={"setup.timeseries.enabled": False})
-        mock_rollback = mocker.patch("tasks.base.django_transaction.rollback")
-        task = BaseCodecovTask()
-        task._rollback_django()
-        assert mock_rollback.call_args_list == [call()]
-
-    def test_run_success_commits_both_orms(self, mocker, dbsession):
-        mock_django_commit = mocker.patch("tasks.base.BaseCodecovTask._commit_django")
+    def test_run_success_commits_sqlalchemy(self, mocker, dbsession):
         mock_wrap_up = mocker.patch("tasks.base.BaseCodecovTask.wrap_up_dbsession")
-        mock_django_rollback = mocker.patch(
-            "tasks.base.BaseCodecovTask._rollback_django"
-        )
         mock_dbsession_rollback = mocker.patch.object(dbsession, "rollback")
         mock_get_db_session = mocker.patch(
             "tasks.base.get_db_session", return_value=dbsession
@@ -250,17 +217,11 @@ def test_run_success_commits_both_orms(self, mocker, dbsession):
         task = SampleTask()
         task.run()
 
-        assert mock_django_commit.call_args_list == [call()]
         assert mock_wrap_up.call_args_list == [call(dbsession)]
 
-        assert mock_django_rollback.call_count == 0
         assert mock_dbsession_rollback.call_count == 0
 
     def test_run_db_errors_rollback(self, mocker, dbsession, celery_app):
-        mock_django_commit = mocker.patch("tasks.base.BaseCodecovTask._commit_django")
-        mock_django_rollback = mocker.patch(
-            "tasks.base.BaseCodecovTask._rollback_django"
-        )
         mock_dbsession_rollback = mocker.patch.object(dbsession, "rollback")
         mock_wrap_up = mocker.patch("tasks.base.BaseCodecovTask.wrap_up_dbsession")
         mock_get_db_session = mocker.patch(
@@ -274,17 +235,11 @@ def test_run_db_errors_rollback(self, mocker, dbsession, celery_app):
         task = celery_app.tasks[registered_task.name]
         task.apply()
 
-        assert mock_django_rollback.call_args_list == [call()]
         assert mock_dbsession_rollback.call_args_list == [call()]
 
-        assert mock_django_commit.call_args_list == [call()]
         assert mock_wrap_up.call_args_list == [call(dbsession)]
 
     def test_run_sqlalchemy_error_rollback(self, mocker, dbsession, celery_app):
-        mock_django_commit = mocker.patch("tasks.base.BaseCodecovTask._commit_django")
-        mock_django_rollback = mocker.patch(
-            "tasks.base.BaseCodecovTask._rollback_django"
-        )
         mock_dbsession_rollback = mocker.patch.object(dbsession, "rollback")
         mock_wrap_up = mocker.patch("tasks.base.BaseCodecovTask.wrap_up_dbsession")
         mock_get_db_session = mocker.patch(
@@ -299,10 +254,8 @@ def test_run_sqlalchemy_error_rollback(self, mocker, dbsession, celery_app):
         task = celery_app.tasks[registered_task.name]
         task.apply()
 
-        assert mock_django_rollback.call_args_list == [call()]
         assert mock_dbsession_rollback.call_args_list == [call()]
 
-        assert mock_django_commit.call_args_list == [call()]
         assert mock_wrap_up.call_args_list == [call(dbsession)]
 
     def test_get_repo_provider_service_working(self, mocker):