From 79074cae46e938f45d370da47be890c1498b0a5c Mon Sep 17 00:00:00 2001 From: RaviTeja799 Date: Mon, 5 Jan 2026 06:29:52 +0530 Subject: [PATCH 01/11] refactor: implement thread-safe get_root with deterministic ordering --- api_app/models.py | 19 +++++- tests/api_app/test_models.py | 116 +++++++++++++++++++++++++++++++++++ 2 files changed, 132 insertions(+), 3 deletions(-) diff --git a/api_app/models.py b/api_app/models.py index 95ecc65668..327dd5df41 100644 --- a/api_app/models.py +++ b/api_app/models.py @@ -457,9 +457,22 @@ def get_root(self): try: return super().get_root() except self.MultipleObjectsReturned: - # django treebeard is not thread safe - # this is not a really valid solution, but it will work for now - return self.objects.filter(path=self.path[0 : self.steplen]).first() # noqa + # django-treebeard is not thread safe - this indicates a data integrity issue. + # We intentionally avoid select_for_update() to prevent potential database + # deadlocks in high-concurrency environments (e.g., multiple Celery workers). + # Using order_by('pk').first() ensures all concurrent requests get the same + # deterministic root node, even if multiple roots exist due to a race condition. + # Note: The root cause is in django-treebeard's tree modification operations, + # not in this read operation. + root_node = type(self).objects.filter( + path=self.path[0 : self.steplen] + ).order_by("pk").first() + logger.error( + f"Tree Integrity Error: Multiple roots found for Job {self.pk} " + f"(path: {self.path}). Returning deterministic root " + f"(PK: {root_node.pk if root_node else 'None'}) as fallback." + ) + return root_node @cached_property def is_sample(self) -> bool: diff --git a/tests/api_app/test_models.py b/tests/api_app/test_models.py index 06208f9ed1..bfbe3f9001 100644 --- a/tests/api_app/test_models.py +++ b/tests/api_app/test_models.py @@ -547,3 +547,119 @@ def test_pivots_to_execute(self): self.assertCountEqual( j1.pivots_to_execute.filter(name="test").values_list("pk", flat=True), [] ) + + + def test_get_root_returns_self_when_is_root(self): + """Test that get_root() returns self when the job is already a root node.""" + an = Analyzable.objects.create( + name="test.com", + classification=Classification.DOMAIN, + ) + root_job = Job.objects.create( + user=self.user, + analyzable=an, + status=Job.STATUSES.REPORTED_WITHOUT_FAILS, + ) + # A newly created job should be a root + self.assertTrue(root_job.is_root()) + self.assertEqual(root_job.get_root(), root_job) + root_job.delete() + an.delete() + + def test_get_root_returns_parent_for_child_job(self): + """Test that get_root() returns the root job for a child job.""" + an = Analyzable.objects.create( + name="test.com", + classification=Classification.DOMAIN, + ) + root_job = Job.add_root( + user=self.user, + analyzable=an, + status=Job.STATUSES.REPORTED_WITHOUT_FAILS, + ) + child_job = root_job.add_child( + user=self.user, + analyzable=an, + status=Job.STATUSES.REPORTED_WITHOUT_FAILS, + ) + # Child job should return the root job + self.assertFalse(child_job.is_root()) + self.assertEqual(child_job.get_root().pk, root_job.pk) + child_job.delete() + root_job.delete() + an.delete() + + def test_get_root_deterministic_ordering(self): + """Test that get_root() returns deterministic results using order_by('pk').""" + an = Analyzable.objects.create( + name="test.com", + classification=Classification.DOMAIN, + ) + root_job = Job.add_root( + user=self.user, + analyzable=an, + status=Job.STATUSES.REPORTED_WITHOUT_FAILS, + ) + # Call get_root multiple times and verify consistent results + results = [root_job.get_root().pk for _ in range(10)] + self.assertEqual(len(set(results)), 1, "get_root() should return consistent results") + root_job.delete() + an.delete() + + def test_get_root_handles_multiple_roots_deterministically(self): + """ + Test that get_root() handles corrupted tree data (multiple roots with same path) + by returning a deterministic result based on PK ordering. + + This simulates the race condition that can occur with django-treebeard + under high concurrency, where multiple root nodes may exist with the same path. + """ + an = Analyzable.objects.create( + name="test.com", + classification=Classification.DOMAIN, + ) + # Create a legitimate root job first + root_job = Job.add_root( + user=self.user, + analyzable=an, + status=Job.STATUSES.REPORTED_WITHOUT_FAILS, + ) + root_path = root_job.path + + # Manually create a duplicate root with the same path to simulate corruption + # This bypasses treebeard's normal creation to force the error condition + duplicate_root = Job.objects.create( + user=self.user, + analyzable=an, + status=Job.STATUSES.REPORTED_WITHOUT_FAILS, + path=root_path, # Same path as original root + depth=root_job.depth, + ) + + # Create a child job under the original root + child_job = root_job.add_child( + user=self.user, + analyzable=an, + status=Job.STATUSES.REPORTED_WITHOUT_FAILS, + ) + + # When get_root is called on the child, it should handle the + # MultipleObjectsReturned exception and return the root with lowest PK + with self.assertLogs('api_app.models', level='ERROR') as log_context: + result = child_job.get_root() + + # Verify deterministic result - should always return the job with lowest PK + expected_root = min(root_job, duplicate_root, key=lambda j: j.pk) + self.assertEqual(result.pk, expected_root.pk) + + # Verify error was logged + self.assertTrue( + any("Tree Integrity Error: Multiple roots found" in msg for msg in log_context.output), + "Expected error log about multiple roots" + ) + + # Cleanup + child_job.delete() + duplicate_root.delete() + root_job.delete() + an.delete() From 3f861ea7e54fc899181228300d38fe69b60964ea Mon Sep 17 00:00:00 2001 From: RaviTeja799 Date: Mon, 5 Jan 2026 17:29:48 +0530 Subject: [PATCH 02/11] style: fix Black formatting in get_root() and test file --- api_app/models.py | 25 +++++++++++++++---------- tests/api_app/test_models.py | 28 ++++++++++++++++------------ 2 files changed, 31 insertions(+), 22 deletions(-) diff --git a/api_app/models.py b/api_app/models.py index 327dd5df41..c2f3e0f68c 100644 --- a/api_app/models.py +++ b/api_app/models.py @@ -457,16 +457,21 @@ def get_root(self): try: return super().get_root() except self.MultipleObjectsReturned: - # django-treebeard is not thread safe - this indicates a data integrity issue. - # We intentionally avoid select_for_update() to prevent potential database - # deadlocks in high-concurrency environments (e.g., multiple Celery workers). - # Using order_by('pk').first() ensures all concurrent requests get the same - # deterministic root node, even if multiple roots exist due to a race condition. - # Note: The root cause is in django-treebeard's tree modification operations, - # not in this read operation. - root_node = type(self).objects.filter( - path=self.path[0 : self.steplen] - ).order_by("pk").first() + # django-treebeard is not thread safe - this indicates a data + # integrity issue. We intentionally avoid select_for_update() to + # prevent potential database deadlocks in high-concurrency + # environments (e.g., multiple Celery workers). + # Using order_by('pk').first() ensures all concurrent requests + # get the same deterministic root node, even if multiple roots + # exist due to a race condition. + # Note: The root cause is in django-treebeard's tree modification + # operations, not in this read operation. + root_node = ( + type(self) + .objects.filter(path=self.path[: self.steplen]) + .order_by("pk") + .first() + ) logger.error( f"Tree Integrity Error: Multiple roots found for Job {self.pk} " f"(path: {self.path}). Returning deterministic root " diff --git a/tests/api_app/test_models.py b/tests/api_app/test_models.py index bfbe3f9001..521ca3f83c 100644 --- a/tests/api_app/test_models.py +++ b/tests/api_app/test_models.py @@ -548,7 +548,6 @@ def test_pivots_to_execute(self): j1.pivots_to_execute.filter(name="test").values_list("pk", flat=True), [] ) - def test_get_root_returns_self_when_is_root(self): """Test that get_root() returns self when the job is already a root node.""" an = Analyzable.objects.create( @@ -602,7 +601,9 @@ def test_get_root_deterministic_ordering(self): ) # Call get_root multiple times and verify consistent results results = [root_job.get_root().pk for _ in range(10)] - self.assertEqual(len(set(results)), 1, "get_root() should return consistent results") + self.assertEqual( + len(set(results)), 1, "get_root() should return consistent results" + ) root_job.delete() an.delete() @@ -610,7 +611,7 @@ def test_get_root_handles_multiple_roots_deterministically(self): """ Test that get_root() handles corrupted tree data (multiple roots with same path) by returning a deterministic result based on PK ordering. - + This simulates the race condition that can occur with django-treebeard under high concurrency, where multiple root nodes may exist with the same path. """ @@ -625,7 +626,7 @@ def test_get_root_handles_multiple_roots_deterministically(self): status=Job.STATUSES.REPORTED_WITHOUT_FAILS, ) root_path = root_job.path - + # Manually create a duplicate root with the same path to simulate corruption # This bypasses treebeard's normal creation to force the error condition duplicate_root = Job.objects.create( @@ -635,29 +636,32 @@ def test_get_root_handles_multiple_roots_deterministically(self): path=root_path, # Same path as original root depth=root_job.depth, ) - + # Create a child job under the original root child_job = root_job.add_child( user=self.user, analyzable=an, status=Job.STATUSES.REPORTED_WITHOUT_FAILS, ) - + # When get_root is called on the child, it should handle the # MultipleObjectsReturned exception and return the root with lowest PK - with self.assertLogs('api_app.models', level='ERROR') as log_context: + with self.assertLogs("api_app.models", level="ERROR") as log_context: result = child_job.get_root() - + # Verify deterministic result - should always return the job with lowest PK expected_root = min(root_job, duplicate_root, key=lambda j: j.pk) self.assertEqual(result.pk, expected_root.pk) - + # Verify error was logged self.assertTrue( - any("Tree Integrity Error: Multiple roots found" in msg for msg in log_context.output), - "Expected error log about multiple roots" + any( + "Tree Integrity Error: Multiple roots found" in msg + for msg in log_context.output + ), + "Expected error log about multiple roots", ) - + # Cleanup child_job.delete() duplicate_root.delete() From 1eb21c7d6e1a24dc73017cb5bd079c78967f171d Mon Sep 17 00:00:00 2001 From: RaviTeja799 Date: Mon, 5 Jan 2026 18:52:41 +0530 Subject: [PATCH 03/11] test: fix test to use mock.patch for MultipleObjectsReturned exception --- tests/api_app/test_models.py | 39 +++++++++++++++--------------------- 1 file changed, 16 insertions(+), 23 deletions(-) diff --git a/tests/api_app/test_models.py b/tests/api_app/test_models.py index 521ca3f83c..754fc707c8 100644 --- a/tests/api_app/test_models.py +++ b/tests/api_app/test_models.py @@ -2,6 +2,7 @@ # See the file 'LICENSE' for copying permission. import datetime from json import loads +from unittest.mock import patch from celery._state import get_current_app from celery.canvas import Signature @@ -609,49 +610,42 @@ def test_get_root_deterministic_ordering(self): def test_get_root_handles_multiple_roots_deterministically(self): """ - Test that get_root() handles corrupted tree data (multiple roots with same path) + Test that get_root() handles MultipleObjectsReturned exception by returning a deterministic result based on PK ordering. This simulates the race condition that can occur with django-treebeard under high concurrency, where multiple root nodes may exist with the same path. + We use mocking because treebeard's save() prevents direct path manipulation. """ an = Analyzable.objects.create( name="test.com", classification=Classification.DOMAIN, ) - # Create a legitimate root job first + # Create a root job and child job root_job = Job.add_root( user=self.user, analyzable=an, status=Job.STATUSES.REPORTED_WITHOUT_FAILS, ) - root_path = root_job.path - - # Manually create a duplicate root with the same path to simulate corruption - # This bypasses treebeard's normal creation to force the error condition - duplicate_root = Job.objects.create( - user=self.user, - analyzable=an, - status=Job.STATUSES.REPORTED_WITHOUT_FAILS, - path=root_path, # Same path as original root - depth=root_job.depth, - ) - - # Create a child job under the original root child_job = root_job.add_child( user=self.user, analyzable=an, status=Job.STATUSES.REPORTED_WITHOUT_FAILS, ) - # When get_root is called on the child, it should handle the - # MultipleObjectsReturned exception and return the root with lowest PK - with self.assertLogs("api_app.models", level="ERROR") as log_context: - result = child_job.get_root() + # Mock treebeard's get_root to raise MultipleObjectsReturned + # This simulates the race condition without needing to corrupt the DB + from treebeard.mp_tree import MP_Node + + with patch.object( + MP_Node, "get_root", side_effect=Job.MultipleObjectsReturned() + ): + with self.assertLogs("api_app.models", level="ERROR") as log_context: + result = child_job.get_root() - # Verify deterministic result - should always return the job with lowest PK - expected_root = min(root_job, duplicate_root, key=lambda j: j.pk) - self.assertEqual(result.pk, expected_root.pk) + # Verify we got a result (the fallback query should work) + self.assertIsNotNone(result) + self.assertEqual(result.pk, root_job.pk) # Verify error was logged self.assertTrue( @@ -664,6 +658,5 @@ def test_get_root_handles_multiple_roots_deterministically(self): # Cleanup child_job.delete() - duplicate_root.delete() root_job.delete() an.delete() From eef87ebf6e091e5d319b9ccfa529b69165f3be57 Mon Sep 17 00:00:00 2001 From: RaviTeja799 Date: Mon, 5 Jan 2026 22:15:48 +0530 Subject: [PATCH 04/11] test: use raw SQL to create duplicate root for MultipleObjectsReturned test --- tests/api_app/test_models.py | 55 +++++++++++++++++++++++++++--------- 1 file changed, 42 insertions(+), 13 deletions(-) diff --git a/tests/api_app/test_models.py b/tests/api_app/test_models.py index 754fc707c8..0cb5555831 100644 --- a/tests/api_app/test_models.py +++ b/tests/api_app/test_models.py @@ -2,7 +2,7 @@ # See the file 'LICENSE' for copying permission. import datetime from json import loads -from unittest.mock import patch + from celery._state import get_current_app from celery.canvas import Signature @@ -615,8 +615,10 @@ def test_get_root_handles_multiple_roots_deterministically(self): This simulates the race condition that can occur with django-treebeard under high concurrency, where multiple root nodes may exist with the same path. - We use mocking because treebeard's save() prevents direct path manipulation. + We use raw SQL to bypass treebeard's save() which normally prevents duplicate paths. """ + from django.db import connection + an = Analyzable.objects.create( name="test.com", classification=Classification.DOMAIN, @@ -633,19 +635,44 @@ def test_get_root_handles_multiple_roots_deterministically(self): status=Job.STATUSES.REPORTED_WITHOUT_FAILS, ) - # Mock treebeard's get_root to raise MultipleObjectsReturned - # This simulates the race condition without needing to corrupt the DB - from treebeard.mp_tree import MP_Node - - with patch.object( - MP_Node, "get_root", side_effect=Job.MultipleObjectsReturned() - ): - with self.assertLogs("api_app.models", level="ERROR") as log_context: - result = child_job.get_root() + # Use raw SQL to create a duplicate root with the same path + # This bypasses treebeard's save() logic that normally prevents this + with connection.cursor() as cursor: + cursor.execute( + """ + INSERT INTO api_app_job ( + path, depth, numchild, status, received_request_time, + tlp, scan_mode, sent_to_bi, user_id, analyzable_id, + runtime_configuration + ) + VALUES (%s, %s, %s, %s, NOW(), %s, %s, %s, %s, %s, %s) + RETURNING id + """, + [ + root_job.path, # Same path as original root - creates duplicate + root_job.depth, + 0, + Job.STATUSES.REPORTED_WITHOUT_FAILS.value, + Job.TLP.CLEAR.value, + 1, + False, + self.user.id, + an.id, + "{}", + ], + ) + duplicate_root_id = cursor.fetchone()[0] + + # When get_root is called on the child, it should handle the + # MultipleObjectsReturned exception and return the root with lowest PK + with self.assertLogs("api_app.models", level="ERROR") as log_context: + result = child_job.get_root() # Verify we got a result (the fallback query should work) self.assertIsNotNone(result) - self.assertEqual(result.pk, root_job.pk) + # Should return the job with lowest PK (deterministic ordering) + expected_pk = min(root_job.pk, duplicate_root_id) + self.assertEqual(result.pk, expected_pk) # Verify error was logged self.assertTrue( @@ -656,7 +683,9 @@ def test_get_root_handles_multiple_roots_deterministically(self): "Expected error log about multiple roots", ) - # Cleanup + # Cleanup - use raw SQL for duplicate since it wasn't created via ORM properly + with connection.cursor() as cursor: + cursor.execute("DELETE FROM api_app_job WHERE id = %s", [duplicate_root_id]) child_job.delete() root_job.delete() an.delete() From 1d90d45300075578ce6b4331eaac045cce114273 Mon Sep 17 00:00:00 2001 From: RaviTeja799 Date: Mon, 5 Jan 2026 22:41:40 +0530 Subject: [PATCH 05/11] style: fix isort import formatting --- tests/api_app/test_models.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/api_app/test_models.py b/tests/api_app/test_models.py index 0cb5555831..35a35b63c1 100644 --- a/tests/api_app/test_models.py +++ b/tests/api_app/test_models.py @@ -3,7 +3,6 @@ import datetime from json import loads - from celery._state import get_current_app from celery.canvas import Signature from django.core.exceptions import ValidationError From c6809370c92a155d4deb346b8375e77ed5537b56 Mon Sep 17 00:00:00 2001 From: RaviTeja799 Date: Tue, 6 Jan 2026 18:08:55 +0530 Subject: [PATCH 06/11] test: use mocking to test MultipleObjectsReturned handling in get_root() --- tests/api_app/test_models.py | 58 +++++++++++------------------------- 1 file changed, 17 insertions(+), 41 deletions(-) diff --git a/tests/api_app/test_models.py b/tests/api_app/test_models.py index 35a35b63c1..4256336129 100644 --- a/tests/api_app/test_models.py +++ b/tests/api_app/test_models.py @@ -613,10 +613,10 @@ def test_get_root_handles_multiple_roots_deterministically(self): by returning a deterministic result based on PK ordering. This simulates the race condition that can occur with django-treebeard - under high concurrency, where multiple root nodes may exist with the same path. - We use raw SQL to bypass treebeard's save() which normally prevents duplicate paths. + under high concurrency. We use mocking because the path field has a + UNIQUE constraint in the database, preventing real duplicates. """ - from django.db import connection + from unittest.mock import patch an = Analyzable.objects.create( name="test.com", @@ -634,44 +634,22 @@ def test_get_root_handles_multiple_roots_deterministically(self): status=Job.STATUSES.REPORTED_WITHOUT_FAILS, ) - # Use raw SQL to create a duplicate root with the same path - # This bypasses treebeard's save() logic that normally prevents this - with connection.cursor() as cursor: - cursor.execute( - """ - INSERT INTO api_app_job ( - path, depth, numchild, status, received_request_time, - tlp, scan_mode, sent_to_bi, user_id, analyzable_id, - runtime_configuration - ) - VALUES (%s, %s, %s, %s, NOW(), %s, %s, %s, %s, %s, %s) - RETURNING id - """, - [ - root_job.path, # Same path as original root - creates duplicate - root_job.depth, - 0, - Job.STATUSES.REPORTED_WITHOUT_FAILS.value, - Job.TLP.CLEAR.value, - 1, - False, - self.user.id, - an.id, - "{}", - ], - ) - duplicate_root_id = cursor.fetchone()[0] - - # When get_root is called on the child, it should handle the - # MultipleObjectsReturned exception and return the root with lowest PK - with self.assertLogs("api_app.models", level="ERROR") as log_context: - result = child_job.get_root() + # Verify child_job is not a root (needed for the test to work) + self.assertFalse(child_job.is_root()) + + # Patch treebeard's MP_Node.get_root to raise MultipleObjectsReturned + # This simulates the race condition where multiple roots exist + with patch( + "treebeard.mp_tree.MP_Node.get_root", + side_effect=Job.MultipleObjectsReturned("Multiple roots found"), + ): + with self.assertLogs("api_app.models", level="ERROR") as log_context: + result = child_job.get_root() # Verify we got a result (the fallback query should work) self.assertIsNotNone(result) - # Should return the job with lowest PK (deterministic ordering) - expected_pk = min(root_job.pk, duplicate_root_id) - self.assertEqual(result.pk, expected_pk) + # The fallback query finds root_job (the only actual root) + self.assertEqual(result.pk, root_job.pk) # Verify error was logged self.assertTrue( @@ -682,9 +660,7 @@ def test_get_root_handles_multiple_roots_deterministically(self): "Expected error log about multiple roots", ) - # Cleanup - use raw SQL for duplicate since it wasn't created via ORM properly - with connection.cursor() as cursor: - cursor.execute("DELETE FROM api_app_job WHERE id = %s", [duplicate_root_id]) + # Cleanup child_job.delete() root_job.delete() an.delete() From 5d5a841dea11586ad6161afcd753f8b0b00761d9 Mon Sep 17 00:00:00 2001 From: RaviTeja799 Date: Tue, 6 Jan 2026 18:16:05 +0530 Subject: [PATCH 07/11] test: use mocking to test MultipleObjectsReturned handling in get_root() --- tests/api_app/test_models.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/tests/api_app/test_models.py b/tests/api_app/test_models.py index 4256336129..645a2cc446 100644 --- a/tests/api_app/test_models.py +++ b/tests/api_app/test_models.py @@ -639,12 +639,14 @@ def test_get_root_handles_multiple_roots_deterministically(self): # Patch treebeard's MP_Node.get_root to raise MultipleObjectsReturned # This simulates the race condition where multiple roots exist - with patch( - "treebeard.mp_tree.MP_Node.get_root", - side_effect=Job.MultipleObjectsReturned("Multiple roots found"), + with ( + patch( + "treebeard.mp_tree.MP_Node.get_root", + side_effect=Job.MultipleObjectsReturned("Multiple roots found"), + ), + self.assertLogs("api_app.models", level="ERROR") as log_context, ): - with self.assertLogs("api_app.models", level="ERROR") as log_context: - result = child_job.get_root() + result = child_job.get_root() # Verify we got a result (the fallback query should work) self.assertIsNotNone(result) From 725ee9b79034fffbe9903b17e092b06fc373ba6e Mon Sep 17 00:00:00 2001 From: RaviTeja799 Date: Thu, 8 Jan 2026 18:51:12 +0530 Subject: [PATCH 08/11] fix: use patch.object for proper mocking of super().get_root() call --- tests/api_app/test_models.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/tests/api_app/test_models.py b/tests/api_app/test_models.py index 645a2cc446..3ccb137bfc 100644 --- a/tests/api_app/test_models.py +++ b/tests/api_app/test_models.py @@ -637,11 +637,16 @@ def test_get_root_handles_multiple_roots_deterministically(self): # Verify child_job is not a root (needed for the test to work) self.assertFalse(child_job.is_root()) + # Import MP_Node to patch its get_root method + from treebeard.mp_tree import MP_Node + # Patch treebeard's MP_Node.get_root to raise MultipleObjectsReturned # This simulates the race condition where multiple roots exist + # We use patch.object to patch the method directly on the class with ( - patch( - "treebeard.mp_tree.MP_Node.get_root", + patch.object( + MP_Node, + "get_root", side_effect=Job.MultipleObjectsReturned("Multiple roots found"), ), self.assertLogs("api_app.models", level="ERROR") as log_context, From 0749ef38347b952d2b6e56a6a84351b38aa29eb5 Mon Sep 17 00:00:00 2001 From: RaviTeja799 Date: Thu, 8 Jan 2026 20:00:36 +0530 Subject: [PATCH 09/11] fix: re-enable logging for test as CI disables it via DISABLE_LOGGING_TEST --- tests/api_app/test_models.py | 70 ++++++++++++++++++++---------------- 1 file changed, 40 insertions(+), 30 deletions(-) diff --git a/tests/api_app/test_models.py b/tests/api_app/test_models.py index 3ccb137bfc..3924b2277b 100644 --- a/tests/api_app/test_models.py +++ b/tests/api_app/test_models.py @@ -616,6 +616,7 @@ def test_get_root_handles_multiple_roots_deterministically(self): under high concurrency. We use mocking because the path field has a UNIQUE constraint in the database, preventing real duplicates. """ + import logging from unittest.mock import patch an = Analyzable.objects.create( @@ -640,34 +641,43 @@ def test_get_root_handles_multiple_roots_deterministically(self): # Import MP_Node to patch its get_root method from treebeard.mp_tree import MP_Node - # Patch treebeard's MP_Node.get_root to raise MultipleObjectsReturned - # This simulates the race condition where multiple roots exist - # We use patch.object to patch the method directly on the class - with ( - patch.object( - MP_Node, - "get_root", - side_effect=Job.MultipleObjectsReturned("Multiple roots found"), - ), - self.assertLogs("api_app.models", level="ERROR") as log_context, - ): - result = child_job.get_root() - - # Verify we got a result (the fallback query should work) - self.assertIsNotNone(result) - # The fallback query finds root_job (the only actual root) - self.assertEqual(result.pk, root_job.pk) - - # Verify error was logged - self.assertTrue( - any( - "Tree Integrity Error: Multiple roots found" in msg - for msg in log_context.output - ), - "Expected error log about multiple roots", - ) + # Temporarily re-enable logging (CI disables it via DISABLE_LOGGING_TEST) + # Save the current logging disable level and re-enable + previous_disable_level = logging.root.manager.disable + logging.disable(logging.NOTSET) - # Cleanup - child_job.delete() - root_job.delete() - an.delete() + try: + # Patch treebeard's MP_Node.get_root to raise MultipleObjectsReturned + # This simulates the race condition where multiple roots exist + # We use patch.object to patch the method directly on the class + with ( + patch.object( + MP_Node, + "get_root", + side_effect=Job.MultipleObjectsReturned("Multiple roots found"), + ), + self.assertLogs("api_app.models", level="ERROR") as log_context, + ): + result = child_job.get_root() + + # Verify we got a result (the fallback query should work) + self.assertIsNotNone(result) + # The fallback query finds root_job (the only actual root) + self.assertEqual(result.pk, root_job.pk) + + # Verify error was logged + self.assertTrue( + any( + "Tree Integrity Error: Multiple roots found" in msg + for msg in log_context.output + ), + "Expected error log about multiple roots", + ) + finally: + # Restore the previous logging disable level + logging.disable(previous_disable_level) + + # Cleanup + child_job.delete() + root_job.delete() + an.delete() From 594066488cf56d043c872fb50ba3628a7738da04 Mon Sep 17 00:00:00 2001 From: RaviTeja799 Date: Fri, 9 Jan 2026 07:28:10 +0530 Subject: [PATCH 10/11] fix(tests): use mock logger instead of assertLogs for CI compatibility --- tests/api_app/test_models.py | 68 ++++++++++++++---------------------- 1 file changed, 27 insertions(+), 41 deletions(-) diff --git a/tests/api_app/test_models.py b/tests/api_app/test_models.py index 3924b2277b..081d03d272 100644 --- a/tests/api_app/test_models.py +++ b/tests/api_app/test_models.py @@ -616,7 +616,6 @@ def test_get_root_handles_multiple_roots_deterministically(self): under high concurrency. We use mocking because the path field has a UNIQUE constraint in the database, preventing real duplicates. """ - import logging from unittest.mock import patch an = Analyzable.objects.create( @@ -641,43 +640,30 @@ def test_get_root_handles_multiple_roots_deterministically(self): # Import MP_Node to patch its get_root method from treebeard.mp_tree import MP_Node - # Temporarily re-enable logging (CI disables it via DISABLE_LOGGING_TEST) - # Save the current logging disable level and re-enable - previous_disable_level = logging.root.manager.disable - logging.disable(logging.NOTSET) - - try: - # Patch treebeard's MP_Node.get_root to raise MultipleObjectsReturned - # This simulates the race condition where multiple roots exist - # We use patch.object to patch the method directly on the class - with ( - patch.object( - MP_Node, - "get_root", - side_effect=Job.MultipleObjectsReturned("Multiple roots found"), - ), - self.assertLogs("api_app.models", level="ERROR") as log_context, - ): - result = child_job.get_root() - - # Verify we got a result (the fallback query should work) - self.assertIsNotNone(result) - # The fallback query finds root_job (the only actual root) - self.assertEqual(result.pk, root_job.pk) - - # Verify error was logged - self.assertTrue( - any( - "Tree Integrity Error: Multiple roots found" in msg - for msg in log_context.output - ), - "Expected error log about multiple roots", - ) - finally: - # Restore the previous logging disable level - logging.disable(previous_disable_level) - - # Cleanup - child_job.delete() - root_job.delete() - an.delete() + # Patch treebeard's MP_Node.get_root to raise MultipleObjectsReturned + # and also patch the logger to verify it was called + with ( + patch.object( + MP_Node, + "get_root", + side_effect=Job.MultipleObjectsReturned("Multiple roots found"), + ), + patch("api_app.models.logger") as mock_logger, + ): + result = child_job.get_root() + + # Verify we got a result (the fallback query should work) + self.assertIsNotNone(result) + # The fallback query finds root_job (the only actual root) + self.assertEqual(result.pk, root_job.pk) + + # Verify error was logged (using mock to avoid CI logging disable issues) + mock_logger.error.assert_called_once() + call_args = mock_logger.error.call_args[0][0] + self.assertIn("Tree Integrity Error", call_args) + self.assertIn("Multiple roots found", call_args) + + # Cleanup + child_job.delete() + root_job.delete() + an.delete() From 4f6a679f091edd7dfead33e146292475ac000a99 Mon Sep 17 00:00:00 2001 From: RaviTeja799 Date: Fri, 9 Jan 2026 14:02:55 +0530 Subject: [PATCH 11/11] refactor: change logger.error to logger.warning per review --- api_app/models.py | 2 +- tests/api_app/test_models.py | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/api_app/models.py b/api_app/models.py index c2f3e0f68c..341debd36e 100644 --- a/api_app/models.py +++ b/api_app/models.py @@ -472,7 +472,7 @@ def get_root(self): .order_by("pk") .first() ) - logger.error( + logger.warning( f"Tree Integrity Error: Multiple roots found for Job {self.pk} " f"(path: {self.path}). Returning deterministic root " f"(PK: {root_node.pk if root_node else 'None'}) as fallback." diff --git a/tests/api_app/test_models.py b/tests/api_app/test_models.py index 081d03d272..9e74387341 100644 --- a/tests/api_app/test_models.py +++ b/tests/api_app/test_models.py @@ -657,9 +657,9 @@ def test_get_root_handles_multiple_roots_deterministically(self): # The fallback query finds root_job (the only actual root) self.assertEqual(result.pk, root_job.pk) - # Verify error was logged (using mock to avoid CI logging disable issues) - mock_logger.error.assert_called_once() - call_args = mock_logger.error.call_args[0][0] + # Verify warning was logged (using mock to avoid CI logging disable issues) + mock_logger.warning.assert_called_once() + call_args = mock_logger.warning.call_args[0][0] self.assertIn("Tree Integrity Error", call_args) self.assertIn("Multiple roots found", call_args)