diff --git a/Docs/Tasks.md b/Docs/Tasks.md index ba42580..36808a8 100644 --- a/Docs/Tasks.md +++ b/Docs/Tasks.md @@ -66,6 +66,8 @@ Reference: `Docs/Refactoring.md` for full analysis of each issue. ### 4. Add Path Validation to `_geoip_reader` Initialization +**Status:** Completed. + **Where:** `backend/app/services/geo_service.py` — the `init_geoip()` function (around line 249) sets the module-level `_geoip_reader` without acquiring `_cache_lock`. **Goal:** Add a clear code comment documenting the startup-only assumption: `init_geoip()` must only be called during application startup (from `startup.py`) before request handling begins. Optionally, add an assertion or guard that prevents double-initialization. diff --git a/backend/app/services/geo_service.py b/backend/app/services/geo_service.py index 89df8ab..82621a5 100644 --- a/backend/app/services/geo_service.py +++ b/backend/app/services/geo_service.py @@ -109,6 +109,11 @@ _dirty: set[str] = set() #: Optional MaxMind GeoLite2 reader initialised by :func:`init_geoip`. _geoip_reader: geoip2.database.Reader | None = None +#: Indicates whether :func:`init_geoip` has already been called. +#: This function is startup-only and must not be invoked again while the +#: process is handling requests. +_geoip_initialized: bool = False + #: Lock protecting mutations to the in-memory geo caches. _cache_lock: asyncio.Lock = asyncio.Lock() @@ -230,13 +235,19 @@ async def re_resolve_all( def init_geoip(mmdb_path: str | None) -> None: """Initialise the MaxMind GeoLite2-Country database reader. + This function is startup-only and must be called before request handling + begins. A second initialization attempt is considered a programming error + and raises ``RuntimeError``. + If *mmdb_path* is ``None``, empty, or the file does not exist the fallback is silently disabled — ip-api.com remains the sole resolver. Args: mmdb_path: Absolute path to a ``GeoLite2-Country.mmdb`` file. """ - global _geoip_reader # noqa: PLW0603 + global _geoip_reader, _geoip_initialized # noqa: PLW0603 + if _geoip_initialized: + raise RuntimeError("GeoIP reader already initialised") if not mmdb_path: return from pathlib import Path # noqa: PLC0415 @@ -247,6 +258,7 @@ def init_geoip(mmdb_path: str | None) -> None: log.warning("geoip_mmdb_not_found", path=mmdb_path) return _geoip_reader = geoip2.database.Reader(mmdb_path) + _geoip_initialized = True log.info("geoip_mmdb_loaded", path=mmdb_path) diff --git a/backend/tests/test_services/test_geo_service.py b/backend/tests/test_services/test_geo_service.py index c58bf19..8359b3e 100644 --- a/backend/tests/test_services/test_geo_service.py +++ b/backend/tests/test_services/test_geo_service.py @@ -48,6 +48,31 @@ def _make_session(response_json: dict[str, object], status: int = 200) -> MagicM async def clear_geo_cache() -> None: """Flush the module-level geo cache before every test.""" await geo_service.clear_cache() + geo_service._geoip_reader = None + geo_service._geoip_initialized = False + + +def test_init_geoip_is_startup_only(tmp_path) -> None: + """A second init_geoip() call raises when the reader was already loaded.""" + path = tmp_path / "GeoLite2-Country.mmdb" + path.write_text("dummy") + + with patch("geoip2.database.Reader", MagicMock(name="Reader")) as mock_reader: + geo_service.init_geoip(str(path)) + assert geo_service._geoip_reader is not None + assert geo_service._geoip_initialized is True + + with pytest.raises(RuntimeError, match="already initialised"): + geo_service.init_geoip(str(path)) + + assert mock_reader.call_count == 1 + + +def test_init_geoip_no_path_leaves_reader_uninitialised() -> None: + """No active reader is created when no path is supplied.""" + geo_service.init_geoip("") + assert geo_service._geoip_reader is None + assert geo_service._geoip_initialized is False # ---------------------------------------------------------------------------