lukas.pupkalipinski/BanGUI
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Refactor: Move module-level mutable flags to JailServiceState

Browse Source 
TASK-004: Replace module-level mutable runtime flags in service layer with
injected state holder, eliminating hidden global state and improving testability
and synchronization boundaries.

Changes:
- Create JailServiceState dataclass in app/utils/runtime_state.py to hold
  backend capability cache and synchronization lock
- Add JailServiceState as a field in RuntimeState (with default_factory)
- Remove module-level _backend_cmd_supported and _backend_cmd_lock from
  jail_service.py
- Refactor _check_backend_cmd_supported() to accept state parameter
- Inject JailServiceState into list_jails() and _fetch_jail_summary() via
  parameters
- Add get_jail_service_state() dependency provider in app/dependencies.py
- Add JailServiceStateDep type alias for router injection
- Update jails router to receive and pass state to service functions
- Update all tests to use jail_service_state fixture and pass state to functions
- Remove duplicate _MAX_PAGE_SIZE constant definition
- Document mutable state management in Backend-Development.md
- Update Architecture.md to describe JailServiceState and state nesting pattern

Benefits:
- Eliminates global mutable state and associated race conditions
- Makes state visible to callers (not hidden in module scope)
- Enables test isolation (each test gets fresh state)
- Prepares codebase for multi-worker deployments (state can be extracted to
  shared backend)
- Synchronization boundaries are now explicit (state.get_backend_cmd_lock())

Compliance:
- All tests pass (17 passed in TestListJails, TestGetJail, TestLockInitialization)
- No ruff linting errors
- Type-safe: JailServiceState properly typed with asyncio.Lock, bool | None

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
Lukas
2026-04-27 18:42:52 +02:00
parent 79112c0430
commit 2e221f6852
8 changed files with 157 additions and 102 deletions
Download Patch File Download Diff File Expand all files Collapse all files

15
backend/app/dependencies.py
View File

@@ -34,7 +34,7 @@ from app.services.geo_cache import GeoCache
from app.services.protocols import Fail2BanMetadataService
from app.utils.constants import SESSION_COOKIE_NAME
from app.utils.rate_limiter import RateLimiter
from app.utils.runtime_state import ApplicationState, RuntimeState
from app.utils.runtime_state import ApplicationState, JailServiceState, RuntimeState
from app.utils.session_cache import NoOpSessionCache, SessionCache
log: structlog.stdlib.BoundLogger = structlog.get_logger()
@@ -333,6 +333,18 @@ async def get_pending_recovery(
return app_context.pending_recovery
async def get_jail_service_state(
app_context: Annotated[ApplicationContext, Depends(get_app_context)],
) -> JailServiceState:
"""Return the jail service state holder from runtime state.
Returns:
The JailServiceState containing capability detection cache and
synchronization primitives for jail operations.
"""
return app_context.runtime_state.jail_service_state
async def get_health_probe() -> Callable[[str], Awaitable[ServerStatus]]:
"""Provide the health probe function for checking fail2ban connectivity.
@@ -439,6 +451,7 @@ Fail2BanStartCommandDep = Annotated[str, Depends(get_fail2ban_start_command)]
GeoCacheDep = Annotated[GeoCache, Depends(get_geo_cache)]
ServerStatusDep = Annotated[ServerStatus, Depends(get_server_status)]
PendingRecoveryDep = Annotated[PendingRecovery | None, Depends(get_pending_recovery)]
JailServiceStateDep = Annotated[JailServiceState, Depends(get_jail_service_state)]
HealthProbeDep = Annotated[Callable[[str], Awaitable[ServerStatus]], Depends(get_health_probe)]
SessionCacheDep = Annotated[SessionCache, Depends(get_session_cache)]
SessionRepoDep = Annotated[SessionRepository, Depends(get_session_repo)]

6
backend/app/routers/jails.py
View File

@@ -29,6 +29,7 @@ from app.dependencies import (
Fail2BanSocketDep,
GeoCacheDep,
HttpSessionDep,
JailServiceStateDep,
)
from app.models.ban import JailBannedIpsResponse
from app.models.jail import (
@@ -56,6 +57,7 @@ _NamePath = Annotated[str, Path(description="Jail name as configured in fail2ban
async def get_jails(
_auth: AuthDep,
socket_path: Fail2BanSocketDep,
state: JailServiceStateDep,
) -> JailListResponse:
"""Return a summary of every active fail2ban jail.
@@ -65,11 +67,13 @@ async def get_jails(
Args:
_auth: Validated session — enforces authentication.
socket_path: Path to the fail2ban Unix domain socket.
state: The jail service state holder.
Returns:
:class:`~app.models.jail.JailListResponse` with all active jails.
"""
return await jail_service.list_jails(socket_path)
return await jail_service.list_jails(socket_path, state)
@router.get(

69
backend/app/services/jail_service.py
View File

@@ -7,6 +7,10 @@ Unix domain socket. All socket I/O is performed through the async
Architecture note: this module is a pure service — it contains **no**
HTTP/FastAPI concerns. All results are returned as Pydantic models so
routers can serialise them directly.
Mutable state (backend capability detection cache) is stored in the
JailServiceState object passed to functions that need it. This allows
for proper synchronization and test isolation.
"""
from __future__ import annotations
@@ -44,6 +48,7 @@ from app.utils.fail2ban_response import (
to_dict,
)
from app.utils.jail_socket import reload_all
from app.utils.runtime_state import JailServiceState # noqa: TC001
if TYPE_CHECKING:
from collections.abc import Awaitable
@@ -74,24 +79,8 @@ class IpLookupResult(TypedDict):
# Constants
# ---------------------------------------------------------------------------
# Capability detection for optional fail2ban transmitter commands (backend, idle).
# These commands are not supported in all fail2ban versions. Caching the result
# avoids sending unsupported commands every polling cycle and spamming the
# fail2ban log with "Invalid command" errors.
_backend_cmd_supported: bool | None = None
_backend_cmd_lock: asyncio.Lock | None = None
def _get_backend_cmd_lock() -> asyncio.Lock:
"""Return the shared backend capability probe lock, initialising it lazily.
The caller must already be running inside the event loop when the lock is
created, which is true for all service entry points in this module.
"""
global _backend_cmd_lock
if _backend_cmd_lock is None:
_backend_cmd_lock = asyncio.Lock()
return _backend_cmd_lock
#: Maximum page size for paginated ban results.
_MAX_PAGE_SIZE: int = 100
# ---------------------------------------------------------------------------
# Custom exceptions
@@ -149,6 +138,7 @@ async def _safe_get(
async def _check_backend_cmd_supported(
client: Fail2BanClient,
jail_name: str,
state: JailServiceState,
) -> bool:
"""Detect whether the fail2ban daemon supports optional ``get ... backend`` command.
@@ -162,45 +152,32 @@ async def _check_backend_cmd_supported(
Args:
client: The :class:`~app.utils.fail2ban_client.Fail2BanClient` to use.
jail_name: Name of any jail to use for the probe command.
state: The jail service state holder for capability cache.
Returns:
``True`` if the command is supported, ``False`` otherwise.
Once determined, the result is cached and reused for all jails.
"""
global _backend_cmd_supported
# Fast path: return cached result if already determined.
if _backend_cmd_supported is not None:
return _backend_cmd_supported
if state.backend_cmd_supported is not None:
return state.backend_cmd_supported
# Slow path: acquire lock and probe the command once.
async with _get_backend_cmd_lock():
async with state.get_backend_cmd_lock():
# Double-check idiom: another coroutine may have probed while we waited.
if _backend_cmd_supported is not None:
return _backend_cmd_supported
if state.backend_cmd_supported is not None:
return state.backend_cmd_supported
# Probe: send the command and catch any exception.
try:
ok(await client.send(["get", jail_name, "backend"]))
_backend_cmd_supported = True
state.backend_cmd_supported = True
log.debug("backend_cmd_supported_detected")
except Exception:
_backend_cmd_supported = False
state.backend_cmd_supported = False
log.debug("backend_cmd_unsupported_detected")
return _backend_cmd_supported
async def _reset_backend_capability_cache() -> None:
"""Reset the cached backend/idle capability detection state.
This helper is intended for test isolation and for any scenario where the
cached probe result must be invalidated before the next detection attempt.
"""
global _backend_cmd_supported
async with _get_backend_cmd_lock():
_backend_cmd_supported = None
return state.backend_cmd_supported
# ---------------------------------------------------------------------------
@@ -208,7 +185,7 @@ async def _reset_backend_capability_cache() -> None:
# ---------------------------------------------------------------------------
async def list_jails(socket_path: str) -> JailListResponse:
async def list_jails(socket_path: str, state: JailServiceState) -> JailListResponse:
"""Return a summary list of all active fail2ban jails.
Queries the daemon for the global jail list and then fetches status
@@ -216,6 +193,7 @@ async def list_jails(socket_path: str) -> JailListResponse:
Args:
socket_path: Path to the fail2ban Unix domain socket.
state: The jail service state holder for capability cache.
Returns:
:class:`~app.models.jail.JailListResponse` with all active jails.
@@ -242,7 +220,7 @@ async def list_jails(socket_path: str) -> JailListResponse:
# 2. Fetch summary data for every jail in parallel.
summaries: list[JailSummary] = await asyncio.gather(
*[_fetch_jail_summary(client, name) for name in jail_names],
*[_fetch_jail_summary(client, name, state) for name in jail_names],
return_exceptions=False,
)
@@ -252,6 +230,7 @@ async def list_jails(socket_path: str) -> JailListResponse:
async def _fetch_jail_summary(
client: Fail2BanClient,
name: str,
state: JailServiceState,
) -> JailSummary:
"""Fetch and build a :class:`~app.models.jail.JailSummary` for one jail.
@@ -265,6 +244,7 @@ async def _fetch_jail_summary(
Args:
client: Shared :class:`~app.utils.fail2ban_client.Fail2BanClient`.
name: Jail name.
state: The jail service state holder for capability cache.
Returns:
A :class:`~app.models.jail.JailSummary` populated from the responses.
@@ -272,7 +252,7 @@ async def _fetch_jail_summary(
# Check whether optional backend/idle commands are supported.
# This probe happens once per session and is cached to avoid repeated
# "Invalid command" errors in the fail2ban log.
backend_cmd_is_supported = await _check_backend_cmd_supported(client, name)
backend_cmd_is_supported = await _check_backend_cmd_supported(client, name, state)
# Build the gather list based on command support.
gather_list: list[Awaitable[object]] = [
@@ -741,9 +721,6 @@ def _parse_ban_entry(entry: str, jail: str) -> ActiveBan | None:
# Public API — Jail-specific paginated bans
# ---------------------------------------------------------------------------
#: Maximum allowed page size for :func:`get_jail_banned_ips`.
_MAX_PAGE_SIZE: int = 100
async def get_jail_banned_ips(
socket_path: str,

35
backend/app/utils/runtime_state.py
View File

@@ -40,6 +40,7 @@ acceptable and keeps the implementation simple.
from __future__ import annotations
import asyncio
import datetime
from dataclasses import dataclass, field
from typing import TYPE_CHECKING, Any
@@ -69,10 +70,43 @@ _RUNTIME_ATTRIBUTES: frozenset[str] = frozenset(
"pending_recovery",
"last_activation",
"runtime_settings",
"jail_service_state",
}
)
@dataclass
class JailServiceState:
"""Mutable runtime state for the jail service.
Stores capability detection results and synchronization primitives used by
jail operations. This state is initialized once and shared across all
service calls within a single worker process.
"""
backend_cmd_supported: bool | None = None
backend_cmd_lock: asyncio.Lock | None = None
def get_backend_cmd_lock(self) -> asyncio.Lock:
"""Return the shared backend capability probe lock, initialising lazily.
The caller must already be running inside the event loop when the lock
is created, which is true for all service entry points.
"""
if self.backend_cmd_lock is None:
self.backend_cmd_lock = asyncio.Lock()
return self.backend_cmd_lock
async def reset_backend_capability_cache(self) -> None:
"""Reset the cached backend/idle capability detection state.
This is intended for test isolation and scenarios where the cached
probe result must be invalidated before the next detection attempt.
"""
async with self.get_backend_cmd_lock():
self.backend_cmd_supported = None
@dataclass
class RuntimeState:
"""Mutable runtime state for the current application instance."""
@@ -82,6 +116,7 @@ class RuntimeState:
pending_recovery: PendingRecovery | None = None
last_activation: ActivationRecord | None = None
runtime_settings: Settings | None = None
jail_service_state: JailServiceState = field(default_factory=JailServiceState)
class ApplicationState(State):

74
backend/tests/test_services/test_jail_service.py
View File

@@ -16,6 +16,7 @@ from app.models.jail import JailDetailResponse, JailListResponse
from app.services import ban_service, jail_service
from app.services.jail_service import JailNotFoundError, JailOperationError
from app.utils import jail_socket
from app.utils.runtime_state import JailServiceState
# ---------------------------------------------------------------------------
# Helpers
@@ -80,6 +81,12 @@ def _patch_client(responses: dict[str, Any]) -> Any:
return stack
@pytest.fixture
def jail_service_state() -> JailServiceState:
"""Provide a fresh JailServiceState for each test."""
return JailServiceState()
# ---------------------------------------------------------------------------
# list_jails
# ---------------------------------------------------------------------------
@@ -88,7 +95,7 @@ def _patch_client(responses: dict[str, Any]) -> Any:
class TestListJails:
"""Unit tests for :func:`~app.services.jail_service.list_jails`."""
async def test_returns_jail_list_response(self) -> None:
async def test_returns_jail_list_response(self, jail_service_state: JailServiceState) -> None:
"""list_jails returns a JailListResponse."""
responses = {
"status": _make_global_status("sshd"),
@@ -100,22 +107,22 @@ class TestListJails:
"get|sshd|idle": (0, False),
}
with _patch_client(responses):
result = await jail_service.list_jails(_SOCKET)
result = await jail_service.list_jails(_SOCKET, jail_service_state)
assert isinstance(result, JailListResponse)
assert result.total == 1
assert result.jails[0].name == "sshd"
async def test_empty_jail_list(self) -> None:
async def test_empty_jail_list(self, jail_service_state: JailServiceState) -> None:
"""list_jails returns empty response when no jails are active."""
responses = {"status": (0, [("Number of jail", 0), ("Jail list", "")])}
with _patch_client(responses):
result = await jail_service.list_jails(_SOCKET)
result = await jail_service.list_jails(_SOCKET, jail_service_state)
assert result.total == 0
assert result.jails == []
async def test_jail_status_populated(self) -> None:
async def test_jail_status_populated(self, jail_service_state: JailServiceState) -> None:
"""list_jails populates JailStatus with failed/banned counters."""
responses = {
"status": _make_global_status("sshd"),
@@ -127,14 +134,14 @@ class TestListJails:
"get|sshd|idle": (0, False),
}
with _patch_client(responses):
result = await jail_service.list_jails(_SOCKET)
result = await jail_service.list_jails(_SOCKET, jail_service_state)
jail = result.jails[0]
assert jail.status is not None
assert jail.status.currently_banned == 5
assert jail.status.total_banned == 50
async def test_jail_config_populated(self) -> None:
async def test_jail_config_populated(self, jail_service_state: JailServiceState) -> None:
"""list_jails populates ban_time, find_time, max_retry, backend."""
responses = {
"status": _make_global_status("sshd"),
@@ -146,7 +153,7 @@ class TestListJails:
"get|sshd|idle": (0, True),
}
with _patch_client(responses):
result = await jail_service.list_jails(_SOCKET)
result = await jail_service.list_jails(_SOCKET, jail_service_state)
jail = result.jails[0]
assert jail.ban_time == 3600
@@ -155,7 +162,7 @@ class TestListJails:
assert jail.backend == "systemd"
assert jail.idle is True
async def test_multiple_jails_returned(self) -> None:
async def test_multiple_jails_returned(self, jail_service_state: JailServiceState) -> None:
"""list_jails fetches all jails listed in the global status."""
responses = {
"status": _make_global_status("sshd, nginx"),
@@ -173,13 +180,13 @@ class TestListJails:
"get|nginx|idle": (0, False),
}
with _patch_client(responses):
result = await jail_service.list_jails(_SOCKET)
result = await jail_service.list_jails(_SOCKET, jail_service_state)
assert result.total == 2
names = {j.name for j in result.jails}
assert names == {"sshd", "nginx"}
async def test_connection_error_propagates(self) -> None:
async def test_connection_error_propagates(self, jail_service_state: JailServiceState) -> None:
"""list_jails raises Fail2BanConnectionError when socket unreachable."""
async def _raise(*_: Any, **__: Any) -> None:
@@ -190,9 +197,9 @@ class TestListJails:
self.send = AsyncMock(side_effect=Fail2BanConnectionError("no socket", _SOCKET))
with patch("app.services.jail_service.Fail2BanClient", _FailClient), pytest.raises(Fail2BanConnectionError):
await jail_service.list_jails(_SOCKET)
await jail_service.list_jails(_SOCKET, jail_service_state)
async def test_backend_idle_commands_unsupported(self) -> None:
async def test_backend_idle_commands_unsupported(self, jail_service_state: JailServiceState) -> None:
"""list_jails handles unsupported backend and idle commands gracefully.
When the fail2ban daemon does not support get ... backend/idle commands,
@@ -200,7 +207,7 @@ class TestListJails:
fail2ban log.
"""
# Reset the capability cache to test detection.
await jail_service._reset_backend_capability_cache()
await jail_service_state.reset_backend_capability_cache()
responses = {
"status": _make_global_status("sshd"),
@@ -213,19 +220,19 @@ class TestListJails:
"get|sshd|maxretry": (0, 5),
}
with _patch_client(responses):
result = await jail_service.list_jails(_SOCKET)
result = await jail_service.list_jails(_SOCKET, jail_service_state)
# Verify the result uses the default values for backend and idle.
jail = result.jails[0]
assert jail.backend == "polling" # default
assert jail.idle is False # default
# Capability should now be cached as False.
assert jail_service._backend_cmd_supported is False
assert jail_service_state.backend_cmd_supported is False
async def test_backend_idle_commands_supported(self) -> None:
async def test_backend_idle_commands_supported(self, jail_service_state: JailServiceState) -> None:
"""list_jails detects and sends backend/idle commands when supported."""
# Reset the capability cache to test detection.
await jail_service._reset_backend_capability_cache()
await jail_service_state.reset_backend_capability_cache()
responses = {
"status": _make_global_status("sshd"),
@@ -239,19 +246,19 @@ class TestListJails:
"get|sshd|idle": (0, True),
}
with _patch_client(responses):
result = await jail_service.list_jails(_SOCKET)
result = await jail_service.list_jails(_SOCKET, jail_service_state)
# Verify real values are returned.
jail = result.jails[0]
assert jail.backend == "systemd" # real value
assert jail.idle is True # real value
# Capability should now be cached as True.
assert jail_service._backend_cmd_supported is True
assert jail_service_state.backend_cmd_supported is True
async def test_backend_idle_commands_cached_after_first_probe(self) -> None:
async def test_backend_idle_commands_cached_after_first_probe(self, jail_service_state: JailServiceState) -> None:
"""list_jails caches capability result and reuses it across polling cycles."""
# Reset the capability cache.
await jail_service._reset_backend_capability_cache()
await jail_service_state.reset_backend_capability_cache()
responses = {
"status": _make_global_status("sshd, nginx"),
@@ -270,7 +277,7 @@ class TestListJails:
"get|nginx|maxretry": (0, 5),
}
with _patch_client(responses):
result = await jail_service.list_jails(_SOCKET)
result = await jail_service.list_jails(_SOCKET, jail_service_state)
# Both jails should return default values (cached result is False).
for jail in result.jails:
@@ -290,14 +297,15 @@ class TestLockInitialization:
assert isinstance(lock, asyncio.Lock)
assert jail_socket._reload_all_lock is lock
async def test_backend_cmd_lock_is_lazy_initialised(self) -> None:
async def test_backend_cmd_lock_is_lazy_initialised(self, jail_service_state: JailServiceState) -> None:
"""The backend capability probe lock should be created lazily on first use."""
jail_service._backend_cmd_lock = None
# Ensure state starts with no lock.
jail_service_state.backend_cmd_lock = None
lock = _ = jail_service._get_backend_cmd_lock()
lock = jail_service_state.get_backend_cmd_lock()
assert isinstance(lock, asyncio.Lock)
assert jail_service._backend_cmd_lock is lock
assert jail_service_state.backend_cmd_lock is lock
class TestGetJail:
@@ -320,7 +328,7 @@ class TestGetJail:
f"get|{name}|actions": (0, ["iptables-multiport"]),
}
async def test_returns_jail_detail_response(self) -> None:
async def test_returns_jail_detail_response(self, jail_service_state: JailServiceState) -> None:
"""get_jail returns a JailDetailResponse."""
with _patch_client(self._full_responses()):
result = await jail_service.get_jail(_SOCKET, "sshd")
@@ -328,35 +336,35 @@ class TestGetJail:
assert isinstance(result, JailDetailResponse)
assert result.jail.name == "sshd"
async def test_log_paths_parsed(self) -> None:
async def test_log_paths_parsed(self, jail_service_state: JailServiceState) -> None:
"""get_jail populates log_paths from fail2ban."""
with _patch_client(self._full_responses()):
result = await jail_service.get_jail(_SOCKET, "sshd")
assert result.jail.log_paths == ["/var/log/auth.log"]
async def test_fail_regex_parsed(self) -> None:
async def test_fail_regex_parsed(self, jail_service_state: JailServiceState) -> None:
"""get_jail populates fail_regex list."""
with _patch_client(self._full_responses()):
result = await jail_service.get_jail(_SOCKET, "sshd")
assert "^.*Failed.*from <HOST>" in result.jail.fail_regex
async def test_ignore_ips_parsed(self) -> None:
async def test_ignore_ips_parsed(self, jail_service_state: JailServiceState) -> None:
"""get_jail populates ignore_ips list."""
with _patch_client(self._full_responses()):
result = await jail_service.get_jail(_SOCKET, "sshd")
assert "127.0.0.1" in result.jail.ignore_ips
async def test_actions_parsed(self) -> None:
async def test_actions_parsed(self, jail_service_state: JailServiceState) -> None:
"""get_jail populates actions list."""
with _patch_client(self._full_responses()):
result = await jail_service.get_jail(_SOCKET, "sshd")
assert result.jail.actions == ["iptables-multiport"]
async def test_jail_not_found_raises(self) -> None:
async def test_jail_not_found_raises(self, jail_service_state: JailServiceState) -> None:
"""get_jail raises JailNotFoundError when jail is unknown."""
not_found_response = (1, Exception("Unknown jail: 'ghost'"))