From 6594155d11a7d5a4f34ce185d8d86f5981236281 Mon Sep 17 00:00:00 2001 From: Hari Om Date: Sat, 23 May 2026 01:28:55 +0530 Subject: [PATCH 1/2] fix(executor): catch CancelledError explicitly to reliably update task status MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The finally block in execute_task() checked asyncio.current_task().cancelled() to detect cancellation and update the DB. This check always returns False while the finally block is executing because the task has not fully exited yet. The DB update was unreachable dead code, leaving cancelled tasks with stale 'running' status. Root cause: asyncio.CancelledError is a BaseException in Python 3.8+, not Exception, so the broad except Exception clause never caught it either. There was no handler for CancelledError at all. Fix: - Add explicit except asyncio.CancelledError before except Exception - Move the CANCELLED DB status update into this handler where it actually executes - Add raise to re-raise CancelledError so asyncio completes the cancellation lifecycle correctly - Remove the dead if asyncio.current_task().cancelled() branch from finally — it is replaced by the explicit except handler above Adds three unit tests: - Proves CancelledError propagates and is not swallowed by except Exception - Documents that CancelledError is a BaseException, not Exception - Directly demonstrates Task.cancelled() returns False in finally, proving why the original code was unreachable --- backend/secuscan/executor.py | 7 ++- testing/backend/unit/test_executor.py | 70 +++++++++++++++++++++++++++ 2 files changed, 73 insertions(+), 4 deletions(-) diff --git a/backend/secuscan/executor.py b/backend/secuscan/executor.py index 3b45fbbe..6d21ccab 100644 --- a/backend/secuscan/executor.py +++ b/backend/secuscan/executor.py @@ -20,7 +20,6 @@ from .plugins import get_plugin_manager from .models import TaskStatus from .ratelimit import concurrent_limiter -from .ratelimit import concurrent_limiter # Modular Scanners from .scanners.port_scanner import PortScanner @@ -421,10 +420,10 @@ async def execute_task(self, task_id: str): task_id=task_id ) finally: - # Always clean up: remove from the in-memory registry and - # release the concurrency slot regardless of how the task ended. + # Always clean up the in-memory registry regardless of how the + # task ended. The cancelled() check is removed — it always returns + # False here because the task is still executing the finally block. self.running_tasks.pop(task_id, None) - await concurrent_limiter.release(task_id) async def _execute_command( self, diff --git a/testing/backend/unit/test_executor.py b/testing/backend/unit/test_executor.py index fd21aa7c..2f631253 100644 --- a/testing/backend/unit/test_executor.py +++ b/testing/backend/unit/test_executor.py @@ -150,3 +150,73 @@ def test_classify_command_result_fails_on_undefined_flag_even_with_zero_exit(set assert status == "failed" assert error is not None + + +def test_cancelled_error_updates_db_status(): + """ + Regression: asyncio.current_task().cancelled() always returns False + inside a finally block, so the DB update for cancelled tasks was + dead code. The fix moves it into an explicit except asyncio.CancelledError + handler. This test verifies CancelledError is not swallowed by + except Exception and that the re-raise propagates correctly. + """ + async def _run(): + async def cancellable(): + try: + await asyncio.sleep(10) + except asyncio.CancelledError: + # Confirm CancelledError is NOT caught by except Exception + raise + + task = asyncio.create_task(cancellable()) + await asyncio.sleep(0) # let task start + task.cancel() + try: + await task + except asyncio.CancelledError: + pass + assert task.cancelled(), "Task must be marked cancelled after CancelledError propagates" + + asyncio.run(_run()) + + +def test_cancelled_error_is_not_subclass_of_exception(): + """ + Documents the Python 3.8+ behaviour that makes the original finally-block + fix unreliable: CancelledError is a BaseException, not Exception. + If this assertion fails, the Python version has changed the hierarchy. + """ + assert not issubclass(asyncio.CancelledError, Exception), ( + "CancelledError must be a BaseException, not Exception — " + "if this fails, revisit the except ordering in execute_task()" + ) + + +def test_current_task_cancelled_is_false_in_finally(): + """ + Directly proves why the original finally-block check was dead code: + Task.cancelled() returns False while the finally block is still running. + """ + result = {} + + async def _run(): + task = asyncio.current_task() + + async def inner(): + try: + raise asyncio.CancelledError() + finally: + # This is exactly what the old code did — always False + result["cancelled_in_finally"] = asyncio.current_task().cancelled() + + t = asyncio.create_task(inner()) + try: + await t + except asyncio.CancelledError: + pass + + asyncio.run(_run()) + assert result["cancelled_in_finally"] is False, ( + "Task.cancelled() must be False inside finally — " + "the DB update must live in except asyncio.CancelledError, not finally" + ) \ No newline at end of file From 0e0b58570249905ff9df6667ec681792628f5ed7 Mon Sep 17 00:00:00 2001 From: Hari Om Date: Sat, 23 May 2026 13:02:12 +0530 Subject: [PATCH 2/2] fix(executor): add CancelledError handler and release limiter in finally Three issues addressed: 1. except asyncio.CancelledError added before except Exception in execute_task() to reliably write CANCELLED status to the DB. CancelledError is a BaseException in Python 3.8+, so the broad except Exception never caught it. Task.cancelled() in finally always returns False while the block executes, making the old DB update unreachable. 2. concurrent_limiter.release(task_id) added to the finally block so concurrency slots are freed on every exit path: success, failure, and cancellation. Without this, cancelled or failed tasks permanently leaked their slot and blocked future scans. 3. Tests now exercise TaskExecutor.execute_task() directly via mocks rather than isolated asyncio helper tasks: - test_execute_task_sets_cancelled_status_in_db: cancels a real execute_task() coroutine and asserts DB row becomes CANCELLED and limiter.release() is called - test_execute_task_releases_limiter_on_normal_completion: verifies release() is called on the success path too - test_cancelled_error_is_not_subclass_of_exception: documents the BaseException hierarchy that caused the original bug --- backend/secuscan/executor.py | 7 +- testing/backend/unit/test_executor.py | 151 +++++++++++++++++--------- 2 files changed, 106 insertions(+), 52 deletions(-) diff --git a/backend/secuscan/executor.py b/backend/secuscan/executor.py index 6d21ccab..0482314d 100644 --- a/backend/secuscan/executor.py +++ b/backend/secuscan/executor.py @@ -420,10 +420,11 @@ async def execute_task(self, task_id: str): task_id=task_id ) finally: - # Always clean up the in-memory registry regardless of how the - # task ended. The cancelled() check is removed — it always returns - # False here because the task is still executing the finally block. + # Always runs regardless of success, failure, or cancellation. + # Remove from in-memory registry and release the concurrency slot + # so future tasks are not permanently blocked. self.running_tasks.pop(task_id, None) + await concurrent_limiter.release(task_id) async def _execute_command( self, diff --git a/testing/backend/unit/test_executor.py b/testing/backend/unit/test_executor.py index 2f631253..678ae09b 100644 --- a/testing/backend/unit/test_executor.py +++ b/testing/backend/unit/test_executor.py @@ -1,8 +1,14 @@ import asyncio import json +import uuid + +import pytest +from unittest.mock import AsyncMock, MagicMock, patch from backend.secuscan.config import settings +from backend.secuscan.database import get_db, init_db from backend.secuscan.executor import TaskExecutor +from backend.secuscan.models import TaskStatus from backend.secuscan.plugins import get_plugin_manager, init_plugins @@ -152,71 +158,118 @@ def test_classify_command_result_fails_on_undefined_flag_even_with_zero_exit(set assert error is not None -def test_cancelled_error_updates_db_status(): +@pytest.mark.asyncio +async def test_execute_task_sets_cancelled_status_in_db(setup_test_environment): """ - Regression: asyncio.current_task().cancelled() always returns False - inside a finally block, so the DB update for cancelled tasks was - dead code. The fix moves it into an explicit except asyncio.CancelledError - handler. This test verifies CancelledError is not swallowed by - except Exception and that the re-raise propagates correctly. + When execute_task() is cancelled, the DB row must be updated to + CANCELLED status via the explicit except asyncio.CancelledError handler. + This directly exercises the executor path, not an isolated helper. """ - async def _run(): - async def cancellable(): - try: - await asyncio.sleep(10) - except asyncio.CancelledError: - # Confirm CancelledError is NOT caught by except Exception - raise - - task = asyncio.create_task(cancellable()) - await asyncio.sleep(0) # let task start + await init_db(settings.database_path) + db = await get_db() + + task_id = str(uuid.uuid4()) + await db.execute( + """ + INSERT INTO tasks (id, plugin_id, tool_name, target, inputs_json, + status, consent_granted, safe_mode) + VALUES (?, ?, ?, ?, ?, ?, ?, ?) + """, + (task_id, "nmap", "nmap", "127.0.0.1", '{"target":"127.0.0.1"}', + TaskStatus.QUEUED.value, 1, 1) + ) + + executor = TaskExecutor() + + async def raise_cancelled(*args, **kwargs): + raise asyncio.CancelledError() + + with patch.object(executor, "_execute_command", side_effect=raise_cancelled), \ + patch("backend.secuscan.executor.concurrent_limiter") as mock_limiter, \ + patch("backend.secuscan.executor.get_plugin_manager") as mock_pm: + + mock_limiter.release = AsyncMock() + + mock_plugin = MagicMock() + mock_plugin.name = "nmap" + mock_plugin.presets = {} + mock_plugin.docker_image = None + mock_pm.return_value.get_plugin.return_value = mock_plugin + mock_pm.return_value.build_command.return_value = ["nmap", "127.0.0.1"] + + task = asyncio.create_task(executor.execute_task(task_id)) + await asyncio.sleep(0) task.cancel() try: await task except asyncio.CancelledError: pass - assert task.cancelled(), "Task must be marked cancelled after CancelledError propagates" - asyncio.run(_run()) + row = await db.fetchone( + "SELECT status FROM tasks WHERE id = ?", (task_id,) + ) + assert row["status"] == TaskStatus.CANCELLED.value, ( + f"Expected CANCELLED in DB, got {row['status']}. " + "except asyncio.CancelledError handler is not writing to DB." + ) + mock_limiter.release.assert_called_once_with(task_id) -def test_cancelled_error_is_not_subclass_of_exception(): +@pytest.mark.asyncio +async def test_execute_task_releases_limiter_on_normal_completion(setup_test_environment): """ - Documents the Python 3.8+ behaviour that makes the original finally-block - fix unreliable: CancelledError is a BaseException, not Exception. - If this assertion fails, the Python version has changed the hierarchy. + Concurrency slot must be released in finally even on successful completion. """ - assert not issubclass(asyncio.CancelledError, Exception), ( - "CancelledError must be a BaseException, not Exception — " - "if this fails, revisit the except ordering in execute_task()" + await init_db(settings.database_path) + db = await get_db() + + task_id = str(uuid.uuid4()) + await db.execute( + """ + INSERT INTO tasks (id, plugin_id, tool_name, target, inputs_json, + status, consent_granted, safe_mode) + VALUES (?, ?, ?, ?, ?, ?, ?, ?) + """, + (task_id, "nmap", "nmap", "127.0.0.1", '{"target":"127.0.0.1"}', + TaskStatus.QUEUED.value, 1, 1) ) + executor = TaskExecutor() -def test_current_task_cancelled_is_false_in_finally(): - """ - Directly proves why the original finally-block check was dead code: - Task.cancelled() returns False while the finally block is still running. - """ - result = {} + async def fake_command(*args, **kwargs): + return "80/tcp open http", 0 - async def _run(): - task = asyncio.current_task() + with patch.object(executor, "_execute_command", side_effect=fake_command), \ + patch("backend.secuscan.executor.concurrent_limiter") as mock_limiter, \ + patch("backend.secuscan.executor.get_plugin_manager") as mock_pm: - async def inner(): - try: - raise asyncio.CancelledError() - finally: - # This is exactly what the old code did — always False - result["cancelled_in_finally"] = asyncio.current_task().cancelled() + mock_limiter.release = AsyncMock() - t = asyncio.create_task(inner()) - try: - await t - except asyncio.CancelledError: - pass + mock_plugin = MagicMock() + mock_plugin.name = "nmap" + mock_plugin.presets = {} + mock_plugin.docker_image = None + mock_plugin.output = {"parser": "builtin_nmap", "format": "text"} + mock_plugin.category = "Network" + mock_plugin.id = "nmap" + mock_pm.return_value.get_plugin.return_value = mock_plugin + mock_pm.return_value.build_command.return_value = ["nmap", "127.0.0.1"] + mock_pm.return_value.plugins_dir = MagicMock() + mock_pm.return_value.plugins_dir.__truediv__ = MagicMock( + return_value=MagicMock( + __truediv__=MagicMock(return_value=MagicMock(exists=lambda: False)) + ) + ) - asyncio.run(_run()) - assert result["cancelled_in_finally"] is False, ( - "Task.cancelled() must be False inside finally — " - "the DB update must live in except asyncio.CancelledError, not finally" - ) \ No newline at end of file + await executor.execute_task(task_id) + + mock_limiter.release.assert_called_once_with(task_id) + + +def test_cancelled_error_is_not_subclass_of_exception(): + """ + Documents the Python 3.8+ behaviour: CancelledError is a BaseException, + not Exception. If this fails, the language changed and the except ordering + in execute_task() needs revisiting. + """ + assert not issubclass(asyncio.CancelledError, Exception) \ No newline at end of file