busya
de023c7a31
Backend: - MigrationPlugin.execute — remove AsyncJobRunner.run() deadlock on post-migration ID sync (2 deadlocks total: plugins/migration.py + api/routes/migration.py trigger_sync_now). Replace blocking runner.run with direct await. - MigrationPlugin.execute — fix temp file leak (dry_run=True prevented cleanup). - MigrationPlugin.execute — IdMappingService(SessionLocal()) now closed in finally. - MigrationPlugin.execute — wire IdMappingService into MigrationEngine constructor so cross-filter patching actually works instead of silently skipping. - MigrationPlugin.execute — SupersetClient.aclose() in finally to prevent httpx connection pool leak. - TaskManager — _async_tasks dict leak (add_done_callback cleanup). - JobLifecycle — CancelledError handler (tasks stuck in RUNNING after cancel). - JobLifecycle — persist_task BEFORE _broadcast_task_status (crash consistency). - JobLifecycle — wait_for_resolution/wait_for_input now have 3600s timeout. - AsyncJobRunner.run — 300s timeout on future.result() (APScheduler thread safety). Translate scheduler: - execute_scheduled_translation — replace blocking runner.run(orch.execute_run(run)) with fire-and-forget TranslationOrchestrator.execute_background(). APScheduler thread is freed in ms instead of blocking for the full translation duration. Translation runs of 10k+ rows (200+ LLM batches, hours) no longer hit the 300s runner timeout. - TranslationOrchestrator.execute_background — new static method: opens own DB session, dispatches asyncio.create_task, handles errors + notification. - Scheduler last_run_at updated at dispatch time (not after completion). Frontend: - MigrationModel.stepReady[3] now requires dryRunResult != null (was always true, allowing UI to reach step 3 without dry-run). - WizardModel.goToStep gate for step 3 uses stepReady[3]. - +page.svelte — dryRunResult no longer self-clears via reactive loop. - Progress bar step 3 indicator gate fixed for new readiness logic. Tests: - 2 new regression tests for migration sync (deadlock-free, completes cleanly). - test_migration_plugin.py — _make_mock_superset_client/_make_mock_mapping_service helpers for proper async mock behavior (aclose, sync_environment AsyncMock). - 10 scheduled-translation tests updated for fire-and-forget pattern. - MigrationModel.test.ts — step 3 invariant + goToStep block test. - All affected tests: 104 backend + 79 frontend = 183 passed.
327 lines
15 KiB
Python
327 lines
15 KiB
Python
# #region TestTranslateSchedulerAudit [C:3] [TYPE Module] [SEMANTICS test,translate,scheduler,audit,async-sync-mismatch,id-mismatch]
|
|
# @BRIEF Orthogonal audit of scheduled translation execution — async/sync mismatch and ID mismatch bugs.
|
|
# @RELATION BINDS_TO -> [execute_scheduled_translation]
|
|
# @RELATION BINDS_TO -> [TranslationScheduler]
|
|
# @RELATION BINDS_TO -> [SchedulerService]
|
|
# @RELATION BINDS_TO -> [AsyncJobRunner]
|
|
# @TEST_EDGE: async_coroutine_not_awaited — execute_run returns coroutine but never awaited
|
|
# @TEST_EDGE: disable_schedule_wrong_id — disable_schedule passes job_id instead of schedule_id
|
|
# @TEST_EDGE: delete_schedule_wrong_id — delete_schedule passes job_id instead of schedule_id
|
|
# @TEST_INVARIANT: execute_run_must_be_awaited -> VERIFIED_BY: [test_scheduler_uses_runner_for_execute_run]
|
|
# @TEST_INVARIANT: remove_job_uses_schedule_id -> VERIFIED_BY: [test_disable_schedule_uses_schedule_id, test_delete_schedule_uses_schedule_id]
|
|
import asyncio
|
|
import inspect
|
|
from pathlib import Path
|
|
from unittest.mock import MagicMock, patch, AsyncMock
|
|
|
|
import pytest
|
|
|
|
sys_path = str(Path(__file__).parent.parent / "src")
|
|
import sys
|
|
if sys_path not in sys.path:
|
|
sys.path.insert(0, sys_path)
|
|
|
|
|
|
# =============================================================================
|
|
# BUG #1 FIX: async/sync mismatch — now uses AsyncJobRunner
|
|
# =============================================================================
|
|
|
|
class TestAsyncSyncMismatch:
|
|
"""Verify that execute_run is async and uses AsyncJobRunner."""
|
|
|
|
# #region test_execute_run_is_async_coroutine [C:2] [TYPE Function]
|
|
# @BRIEF Confirm execute_run is an async method that returns a coroutine.
|
|
def test_execute_run_is_async_coroutine(self):
|
|
"""execute_run is defined as async def — calling it without await returns a coroutine object."""
|
|
from src.plugins.translate.orchestrator import TranslationOrchestrator
|
|
|
|
# Verify the method is actually async
|
|
assert inspect.iscoroutinefunction(TranslationOrchestrator.execute_run), (
|
|
"TranslationOrchestrator.execute_run must be an async method"
|
|
)
|
|
# #endregion test_execute_run_is_async_coroutine
|
|
|
|
# #region test_scheduler_uses_runner_for_execute_run [C:2] [TYPE Function]
|
|
# @BRIEF The scheduler uses execute_background static method (fire-and-forget, no timeout cap).
|
|
def test_scheduler_uses_runner_for_execute_run(self):
|
|
"""In execute_scheduled_translation, TranslationOrchestrator.execute_background is called."""
|
|
import ast
|
|
import inspect
|
|
|
|
# Read the source code of execute_scheduled_translation
|
|
from src.plugins.translate.scheduler import execute_scheduled_translation
|
|
source = inspect.getsource(execute_scheduled_translation)
|
|
|
|
# Parse the AST to find the call to execute_background
|
|
tree = ast.parse(source)
|
|
|
|
# Find all Call nodes that call execute_background
|
|
execute_bg_calls = []
|
|
for node in ast.walk(tree):
|
|
if isinstance(node, ast.Call):
|
|
if isinstance(node.func, ast.Attribute) and node.func.attr == 'execute_background':
|
|
execute_bg_calls.append(node)
|
|
|
|
assert len(execute_bg_calls) > 0, (
|
|
"FIX NOT APPLIED: execute_background should be called in execute_scheduled_translation. "
|
|
"Expected: TranslationOrchestrator.execute_background(run_id=..., ...)"
|
|
)
|
|
|
|
# Confirm there is NO runner.run(orch.execute_run(...)) call
|
|
for node in ast.walk(tree):
|
|
if isinstance(node, ast.Call):
|
|
if isinstance(node.func, ast.Attribute) and node.func.attr == 'run':
|
|
for arg in node.args:
|
|
if isinstance(arg, ast.Call):
|
|
if isinstance(arg.func, ast.Attribute) and arg.func.attr == 'execute_run':
|
|
pytest.fail(
|
|
"REGRESSION: runner.run(orch.execute_run(run)) found. "
|
|
"Should use TranslationOrchestrator.execute_background() instead."
|
|
)
|
|
# #endregion test_scheduler_uses_runner_for_execute_run
|
|
|
|
# #region test_manual_trigger_uses_asyncio_create_task [C:2] [TYPE Function]
|
|
# @BRIEF Manual trigger correctly uses asyncio.create_task for async execution.
|
|
def test_manual_trigger_uses_asyncio_create_task(self):
|
|
"""The manual trigger route uses asyncio.create_task() which correctly handles async execution."""
|
|
import ast
|
|
import inspect
|
|
from src.api.routes.translate._run_routes import run_translation
|
|
|
|
source = inspect.getsource(run_translation)
|
|
tree = ast.parse(source)
|
|
|
|
# Look for asyncio.create_task calls
|
|
has_create_task = False
|
|
for node in ast.walk(tree):
|
|
if isinstance(node, ast.Call):
|
|
if isinstance(node.func, ast.Attribute):
|
|
if node.func.attr == 'create_task':
|
|
has_create_task = True
|
|
elif isinstance(node.func, ast.Name):
|
|
if node.func.id == 'create_task':
|
|
has_create_task = True
|
|
|
|
assert has_create_task, (
|
|
"Manual trigger should use asyncio.create_task() for async execution"
|
|
)
|
|
# #endregion test_manual_trigger_uses_asyncio_create_task
|
|
|
|
|
|
# =============================================================================
|
|
# BUG #2: ID mismatch — disable_schedule and delete_schedule pass wrong ID
|
|
# =============================================================================
|
|
|
|
class TestScheduleIdMismatch:
|
|
"""Verify that disable_schedule and delete_schedule use correct IDs."""
|
|
|
|
# #region test_add_translation_job_uses_schedule_id [C:2] [TYPE Function]
|
|
# @BRIEF add_translation_job constructs APScheduler job ID from schedule_id.
|
|
def test_add_translation_job_uses_schedule_id(self):
|
|
"""add_translation_job creates job ID as 'translate_{schedule_id}'."""
|
|
from src.core.scheduler import SchedulerService
|
|
|
|
mock_task_manager = MagicMock()
|
|
mock_config = MagicMock()
|
|
|
|
svc = SchedulerService(mock_task_manager, mock_config)
|
|
svc.scheduler = MagicMock()
|
|
|
|
schedule_id = "sched-uuid-123"
|
|
job_id = "job-uuid-456"
|
|
|
|
svc.add_translation_job(
|
|
schedule_id=schedule_id,
|
|
job_id=job_id,
|
|
cron_expression="*/30 * * * *",
|
|
timezone="UTC",
|
|
execution_mode="full"
|
|
)
|
|
|
|
# Verify the APScheduler job ID is based on schedule_id
|
|
call_args = svc.scheduler.add_job.call_args
|
|
aps_job_id = call_args.kwargs.get('id') or call_args.args[1] if len(call_args.args) > 1 else None
|
|
|
|
assert aps_job_id == f"translate_{schedule_id}", (
|
|
f"APScheduler job ID should be 'translate_{schedule_id}', got '{aps_job_id}'"
|
|
)
|
|
# #endregion test_add_translation_job_uses_schedule_id
|
|
|
|
# #region test_remove_translation_job_uses_schedule_id [C:2] [TYPE Function]
|
|
# @BRIEF remove_translation_job constructs APScheduler job ID from schedule_id.
|
|
def test_remove_translation_job_uses_schedule_id(self):
|
|
"""remove_translation_job removes job with ID 'translate_{schedule_id}'."""
|
|
from src.core.scheduler import SchedulerService
|
|
|
|
mock_task_manager = MagicMock()
|
|
mock_config = MagicMock()
|
|
|
|
svc = SchedulerService(mock_task_manager, mock_config)
|
|
svc.scheduler = MagicMock()
|
|
|
|
schedule_id = "sched-uuid-123"
|
|
svc.remove_translation_job(schedule_id=schedule_id)
|
|
|
|
# Verify it tries to remove the correct job ID
|
|
svc.scheduler.remove_job.assert_called_once_with(f"translate_{schedule_id}")
|
|
# #endregion test_remove_translation_job_uses_schedule_id
|
|
|
|
# #region test_disable_schedule_uses_schedule_id [C:2] [TYPE Function]
|
|
# @BRIEF disable_schedule uses schedule.id (not job_id) for remove_translation_job (FIXED).
|
|
def test_disable_schedule_uses_schedule_id(self):
|
|
"""FIXED: disable_schedule route uses schedule.id for remove_translation_job."""
|
|
import ast
|
|
import inspect
|
|
from src.api.routes.translate._schedule_routes import disable_schedule
|
|
|
|
source = inspect.getsource(disable_schedule)
|
|
tree = ast.parse(source)
|
|
|
|
# Find calls to remove_translation_job
|
|
remove_calls = []
|
|
for node in ast.walk(tree):
|
|
if isinstance(node, ast.Call):
|
|
if isinstance(node.func, ast.Attribute) and node.func.attr == 'remove_translation_job':
|
|
remove_calls.append(node)
|
|
|
|
assert len(remove_calls) > 0, "No calls to remove_translation_job found"
|
|
|
|
# Check what argument is passed — should NOT be job_id directly
|
|
for call in remove_calls:
|
|
for kw in call.keywords:
|
|
if kw.arg == 'schedule_id':
|
|
# Should NOT be job_id (the parameter name)
|
|
if isinstance(kw.value, ast.Name) and kw.value.id == 'job_id':
|
|
pytest.fail(
|
|
"BUG STILL EXISTS: disable_schedule passes job_id to remove_translation_job. "
|
|
"Expected: schedule.id from set_schedule_active() return value."
|
|
)
|
|
# #endregion test_disable_schedule_uses_schedule_id
|
|
|
|
# #region test_delete_schedule_uses_schedule_id [C:2] [TYPE Function]
|
|
# @BRIEF delete_schedule uses schedule.id (not job_id) for remove_translation_job (FIXED).
|
|
def test_delete_schedule_uses_schedule_id(self):
|
|
"""FIXED: delete_schedule route uses schedule.id for remove_translation_job."""
|
|
import ast
|
|
import inspect
|
|
from src.api.routes.translate._schedule_routes import delete_schedule
|
|
|
|
source = inspect.getsource(delete_schedule)
|
|
tree = ast.parse(source)
|
|
|
|
# Find calls to remove_translation_job
|
|
remove_calls = []
|
|
for node in ast.walk(tree):
|
|
if isinstance(node, ast.Call):
|
|
if isinstance(node.func, ast.Attribute) and node.func.attr == 'remove_translation_job':
|
|
remove_calls.append(node)
|
|
|
|
assert len(remove_calls) > 0, "No calls to remove_translation_job found"
|
|
|
|
# Check what argument is passed — should NOT be job_id directly
|
|
for call in remove_calls:
|
|
for kw in call.keywords:
|
|
if kw.arg == 'schedule_id':
|
|
# Should NOT be job_id (the parameter name)
|
|
if isinstance(kw.value, ast.Name) and kw.value.id == 'job_id':
|
|
pytest.fail(
|
|
"BUG STILL EXISTS: delete_schedule passes job_id to remove_translation_job. "
|
|
"Expected: schedule_id from get_schedule() return value."
|
|
)
|
|
# #endregion test_delete_schedule_uses_schedule_id
|
|
|
|
# #region test_schedule_id_differs_from_job_id [C:2] [TYPE Function]
|
|
# @BRIEF TranslationSchedule.id and TranslationSchedule.job_id are different UUIDs.
|
|
def test_schedule_id_differs_from_job_id(self):
|
|
"""TranslationSchedule has separate id and job_id fields — they are different UUIDs."""
|
|
from src.models.translate import TranslationSchedule
|
|
|
|
# Verify the model has both fields
|
|
assert hasattr(TranslationSchedule, 'id'), "TranslationSchedule should have 'id' field"
|
|
assert hasattr(TranslationSchedule, 'job_id'), "TranslationSchedule should have 'job_id' field"
|
|
|
|
# The id is the primary key, job_id is a foreign key to TranslationJob
|
|
# They are different UUIDs
|
|
import uuid
|
|
schedule_id = str(uuid.uuid4())
|
|
job_id = str(uuid.uuid4())
|
|
|
|
assert schedule_id != job_id, (
|
|
"schedule_id and job_id are different UUIDs — "
|
|
"using job_id where schedule_id is expected will cause ID mismatch"
|
|
)
|
|
# #endregion test_schedule_id_differs_from_job_id
|
|
|
|
|
|
# =============================================================================
|
|
# Integration verification: end-to-end scheduled execution flow
|
|
# =============================================================================
|
|
|
|
class TestScheduledExecutionFlow:
|
|
"""Verify the complete scheduled execution flow has the async/sync bug."""
|
|
|
|
# #region test_scheduled_execution_uses_runner [C:2] [TYPE Function]
|
|
# @BRIEF Full flow: scheduler dispatches via execute_background (fire-and-forget).
|
|
def test_scheduled_execution_uses_runner(self):
|
|
"""
|
|
Simulate what happens when APScheduler triggers execute_scheduled_translation:
|
|
1. TranslationOrchestrator is created
|
|
2. start_run creates a TranslationRun
|
|
3. execute_background is called with the run_id
|
|
4. schedule.last_run_at is updated at dispatch time
|
|
"""
|
|
from src.plugins.translate.scheduler import execute_scheduled_translation
|
|
from src.plugins.translate.orchestrator import TranslationOrchestrator
|
|
|
|
mock_db = MagicMock()
|
|
mock_session_maker = MagicMock(return_value=mock_db)
|
|
mock_config = MagicMock()
|
|
|
|
# Setup schedule
|
|
schedule = MagicMock()
|
|
schedule.id = "sched-1"
|
|
schedule.job_id = "job-1"
|
|
schedule.is_active = True
|
|
schedule.execution_mode = "full"
|
|
|
|
q1 = MagicMock()
|
|
q1.filter.return_value.first.return_value = schedule
|
|
|
|
q2 = MagicMock()
|
|
q2.filter.return_value.order_by.return_value.first.return_value = None # no concurrent
|
|
|
|
q3 = MagicMock()
|
|
q3.filter.return_value.order_by.return_value.first.return_value = None # no recent run
|
|
|
|
mock_db.query.side_effect = [q1, q2, q3]
|
|
|
|
# Setup orchestrator mock
|
|
mock_run = MagicMock()
|
|
mock_run.id = "run-1"
|
|
mock_run.status = "PENDING"
|
|
|
|
# Track execute_background calls — runner mock must execute the dispatch coroutine
|
|
mock_runner = MagicMock()
|
|
mock_runner.run.side_effect = lambda coro: asyncio.run(coro)
|
|
with patch.object(TranslationOrchestrator, 'execute_background') as mock_exec_bg, \
|
|
patch.object(TranslationOrchestrator, 'start_run', return_value=mock_run), \
|
|
patch('src.dependencies.get_async_job_runner', return_value=mock_runner):
|
|
execute_scheduled_translation(
|
|
schedule_id="sched-1",
|
|
job_id="job-1",
|
|
db_session_maker=mock_session_maker,
|
|
config_manager=mock_config,
|
|
execution_mode="full",
|
|
)
|
|
|
|
# Verify that execute_background was called with the correct run_id (NOT execute_run via runner.run)
|
|
mock_exec_bg.assert_called_once_with(
|
|
run_id="run-1",
|
|
db_session_maker=mock_session_maker,
|
|
config_manager=mock_config,
|
|
current_user="scheduler",
|
|
)
|
|
# Schedule tracking is updated at dispatch time
|
|
assert schedule.last_run_at is not None
|
|
# #endregion test_scheduled_execution_uses_runner
|