Fix issue #31: Make schedule reschedule deterministic and observable
Replace fire-and-forget reschedule pattern with proper async/await: - Changed reschedule() from fire-and-forget to awaitable async function - Errors are now properly propagated instead of silently failing - Added structured logging for reschedule start and completion - Schedule updates are now deterministic and observable to callers Changes: - app/tasks/blocklist_import.py: Convert reschedule to async, remove asyncio.ensure_future - tests/test_tasks/test_blocklist_import.py: Add tests for error propagation and logging - Docs/Features.md: Document scheduling reliability guarantees All 15 blocklist_import tests pass with 100% coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
@@ -339,8 +339,9 @@ class TestRegister:
|
||||
class TestReschedule:
|
||||
"""Tests for :func:`~app.tasks.blocklist_import.reschedule`."""
|
||||
|
||||
def test_reschedule_calls_ensure_future(self) -> None:
|
||||
"""``reschedule`` must schedule the re-registration with ``asyncio.ensure_future``."""
|
||||
@pytest.mark.asyncio
|
||||
async def test_reschedule_fetches_and_applies_schedule(self) -> None:
|
||||
"""``reschedule`` must fetch the schedule and apply it synchronously."""
|
||||
from app.tasks.blocklist_import import reschedule
|
||||
|
||||
app = MagicMock()
|
||||
@@ -348,15 +349,80 @@ class TestReschedule:
|
||||
app.state.db.close = AsyncMock()
|
||||
app.state.settings = MagicMock(database_path="/tmp/fake.db")
|
||||
app.state.scheduler = MagicMock()
|
||||
app.state.scheduler.get_job.return_value = None
|
||||
app.state.http_session = MagicMock()
|
||||
app.state.runtime_settings = None
|
||||
|
||||
def _close_coro(coro: Any) -> None:
|
||||
coro.close()
|
||||
config = ScheduleConfig(frequency=ScheduleFrequency.daily, hour=3, minute=0)
|
||||
|
||||
with patch(
|
||||
"app.tasks.db.open_db",
|
||||
new_callable=AsyncMock,
|
||||
return_value=app.state.db,
|
||||
), patch("asyncio.ensure_future", side_effect=_close_coro) as mock_ensure_future:
|
||||
reschedule(app)
|
||||
), patch(
|
||||
"app.tasks.blocklist_import.blocklist_service.get_schedule",
|
||||
new_callable=AsyncMock,
|
||||
return_value=config,
|
||||
), patch(
|
||||
"app.tasks.blocklist_import._apply_schedule"
|
||||
) as mock_apply_schedule:
|
||||
await reschedule(app)
|
||||
|
||||
mock_ensure_future.assert_called_once()
|
||||
mock_apply_schedule.assert_called_once_with(app, config)
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_reschedule_propagates_errors(self) -> None:
|
||||
"""``reschedule`` must propagate exceptions from schedule fetch."""
|
||||
from app.tasks.blocklist_import import reschedule
|
||||
|
||||
app = MagicMock()
|
||||
app.state.db = MagicMock()
|
||||
app.state.db.close = AsyncMock()
|
||||
app.state.settings = MagicMock(database_path="/tmp/fake.db")
|
||||
app.state.runtime_settings = None
|
||||
|
||||
with patch(
|
||||
"app.tasks.db.open_db",
|
||||
new_callable=AsyncMock,
|
||||
return_value=app.state.db,
|
||||
), patch(
|
||||
"app.tasks.blocklist_import.blocklist_service.get_schedule",
|
||||
new_callable=AsyncMock,
|
||||
side_effect=RuntimeError("Database error"),
|
||||
):
|
||||
with pytest.raises(RuntimeError, match="Database error"):
|
||||
await reschedule(app)
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_reschedule_logs_events(self) -> None:
|
||||
"""``reschedule`` must log start and completion events."""
|
||||
from app.tasks.blocklist_import import reschedule
|
||||
|
||||
app = MagicMock()
|
||||
app.state.db = MagicMock()
|
||||
app.state.db.close = AsyncMock()
|
||||
app.state.settings = MagicMock(database_path="/tmp/fake.db")
|
||||
app.state.scheduler = MagicMock()
|
||||
app.state.scheduler.get_job.return_value = None
|
||||
app.state.http_session = MagicMock()
|
||||
app.state.runtime_settings = None
|
||||
|
||||
config = ScheduleConfig(frequency=ScheduleFrequency.daily, hour=3, minute=0)
|
||||
|
||||
with patch(
|
||||
"app.tasks.db.open_db",
|
||||
new_callable=AsyncMock,
|
||||
return_value=app.state.db,
|
||||
), patch(
|
||||
"app.tasks.blocklist_import.blocklist_service.get_schedule",
|
||||
new_callable=AsyncMock,
|
||||
return_value=config,
|
||||
), patch(
|
||||
"app.tasks.blocklist_import._apply_schedule"
|
||||
), patch("app.tasks.blocklist_import.log") as mock_log:
|
||||
await reschedule(app)
|
||||
|
||||
# Check for start and completion log calls
|
||||
log_calls = [c[0][0] for c in mock_log.info.call_args_list]
|
||||
assert "blocklist_reschedule_applying" in log_calls
|
||||
assert "blocklist_reschedule_applied" in log_calls
|
||||
|
||||
Reference in New Issue
Block a user