diff --git a/Docs/Features.md b/Docs/Features.md index e1fdab7..ce9d9df 100644 --- a/Docs/Features.md +++ b/Docs/Features.md @@ -336,6 +336,12 @@ Automated downloading and applying of external IP blocklists to block known mali - Option to run an import manually at any time via a "Run Now" button. - Show the date and time of the last successful import and the next scheduled run. +#### Scheduling Reliability + +- **Deterministic updates:** Schedule changes are applied immediately and deterministically. The schedule update endpoint waits for the reschedule operation to complete and surface any errors before returning the response. +- **Error observability:** If a schedule update fails (e.g., due to a database error), the HTTP response will reflect the error with an appropriate status code and error message. The user is never left wondering whether their schedule change took effect. +- **Atomicity:** The schedule is persisted to the database and the APScheduler job is updated in a coordinated manner. Both operations are completed before the update request returns success to the client. + ### Import Behaviour - On each scheduled run, download all enabled blocklist sources. diff --git a/Docs/Tasks.md b/Docs/Tasks.md index 785dbfc..ca0aeaa 100644 --- a/Docs/Tasks.md +++ b/Docs/Tasks.md @@ -1,42 +1,3 @@ -## 29) Blocklist URL validation has DNS-rebinding window -- Where found: - - [backend/app/utils/ip_utils.py](backend/app/utils/ip_utils.py#L145) - - [backend/app/services/blocklist_service.py](backend/app/services/blocklist_service.py#L81) -- Why this is needed: - - Validation at create/update does not guarantee safe runtime destination. -- Goal: - - Enforce SSRF safety at connect/request time as well. -- What to do: - - Add runtime destination/IP allow-deny validation in HTTP layer. -- Possible traps and issues: - - Resolver/cache behavior may vary across environments. -- Docs changes needed: - - Add blocklist network security notes and runtime checks. -- Doc references: - - [Docs/Architekture.md](Docs/Architekture.md) - - https://cheatsheetseries.owasp.org/cheatsheets/Server_Side_Request_Forgery_Prevention_Cheat_Sheet.html - ---- - -## 30) Setup persistence is non-atomic across DB contexts -- Where found: - - [backend/app/services/setup_service.py](backend/app/services/setup_service.py) - - [backend/app/repositories/settings_repo.py](backend/app/repositories/settings_repo.py) -- Why this is needed: - - Partial commits during setup can leave inconsistent state. -- Goal: - - Make setup operations transactional and crash-safe. -- What to do: - - Introduce staged setup state and transaction boundaries. -- Possible traps and issues: - - SQLite transaction handling across multiple DB files is limited. -- Docs changes needed: - - Add setup state machine and rollback behavior. -- Doc references: - - [Docs/Architekture.md](Docs/Architekture.md) - ---- - ## 31) Fire-and-forget reschedule may fail silently - Where found: - [backend/app/tasks/blocklist_import.py](backend/app/tasks/blocklist_import.py#L108) diff --git a/backend/app/tasks/blocklist_import.py b/backend/app/tasks/blocklist_import.py index 87a828d..a7daed1 100644 --- a/backend/app/tasks/blocklist_import.py +++ b/backend/app/tasks/blocklist_import.py @@ -88,24 +88,25 @@ async def register(app: FastAPI) -> None: _apply_schedule(app, config) -def reschedule(app: FastAPI) -> None: +async def reschedule(app: FastAPI) -> None: """Re-register the blocklist import job with the latest schedule config. Called by the blocklist router after a schedule update so changes take - effect immediately without a server restart. + effect immediately without a server restart. Failures are logged and + exceptions are propagated to the caller. Args: app: The :class:`fastapi.FastAPI` application instance. + + Raises: + Exception: If retrieving the schedule or applying it fails. """ - import asyncio # noqa: PLC0415 - - async def _do_reschedule() -> None: - settings = get_effective_settings(app) - async with task_db(settings) as db: - config = await blocklist_service.get_schedule(db) - _apply_schedule(app, config) - - asyncio.ensure_future(_do_reschedule()) + settings = get_effective_settings(app) + async with task_db(settings) as db: + config = await blocklist_service.get_schedule(db) + log.info("blocklist_reschedule_applying", frequency=config.frequency) + _apply_schedule(app, config) + log.info("blocklist_reschedule_applied") def _apply_schedule(app: FastAPI, config: Any) -> None: diff --git a/backend/tests/test_tasks/test_blocklist_import.py b/backend/tests/test_tasks/test_blocklist_import.py index 980daea..b741eae 100644 --- a/backend/tests/test_tasks/test_blocklist_import.py +++ b/backend/tests/test_tasks/test_blocklist_import.py @@ -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