From ef018b7effb7ac81ab8935060fbe6f14ff285fae Mon Sep 17 00:00:00 2001 From: Aurelio <19254254+Aureliolo@users.noreply.github.com> Date: Sat, 9 May 2026 12:54:29 +0200 Subject: [PATCH 1/5] refactor(api): move activities lifecycle-cap fallback to ApiBridgeConfig snapshot Closes #1797 --- scripts/no_magic_numbers_baseline.txt | 9 +- src/synthorg/api/controllers/activities.py | 50 +----- src/synthorg/api/controllers/audit.py | 7 +- .../api/controllers/coordination_metrics.py | 7 +- src/synthorg/api/lifecycle_helpers.py | 40 +++++ src/synthorg/api/state.py | 11 ++ src/synthorg/api/state_services.py | 36 +++++ src/synthorg/settings/subscribers/__init__.py | 4 + .../subscribers/api_bridge_subscriber.py | 112 +++++++++++++ tests/unit/api/controllers/test_activities.py | 82 ++++++++++ .../test_apply_api_bridge_config_snapshot.py | 103 ++++++++++++ tests/unit/api/test_state.py | 40 +++++ .../settings/test_api_bridge_subscriber.py | 150 ++++++++++++++++++ 13 files changed, 593 insertions(+), 58 deletions(-) create mode 100644 src/synthorg/settings/subscribers/api_bridge_subscriber.py create mode 100644 tests/unit/api/test_apply_api_bridge_config_snapshot.py create mode 100644 tests/unit/settings/test_api_bridge_subscriber.py diff --git a/scripts/no_magic_numbers_baseline.txt b/scripts/no_magic_numbers_baseline.txt index 42c7cb12f3..2c9a01a1b0 100644 --- a/scripts/no_magic_numbers_baseline.txt +++ b/scripts/no_magic_numbers_baseline.txt @@ -33,8 +33,7 @@ src/synthorg/api/auth/token_size.py:30:29 src/synthorg/api/auth/token_size.py:38:29 src/synthorg/api/auth_response_discriminator.py:29:30 src/synthorg/api/boundary.py:46:24 -src/synthorg/api/controllers/activities.py:45:24 -src/synthorg/api/controllers/activities.py:366:29 +src/synthorg/api/controllers/activities.py:318:29 src/synthorg/api/controllers/agent_identity_versions.py:111:29 src/synthorg/api/controllers/agents.py:66:24 src/synthorg/api/controllers/agents.py:190:29 @@ -45,7 +44,7 @@ src/synthorg/api/controllers/approvals.py:75:40 src/synthorg/api/controllers/approvals.py:519:29 src/synthorg/api/controllers/artifacts.py:181:29 src/synthorg/api/controllers/audit.py:39:19 -src/synthorg/api/controllers/audit.py:101:29 +src/synthorg/api/controllers/audit.py:104:29 src/synthorg/api/controllers/backup.py:187:29 src/synthorg/api/controllers/budget.py:302:29 src/synthorg/api/controllers/budget_config_versions.py:46:29 @@ -55,7 +54,7 @@ src/synthorg/api/controllers/connections.py:65:20 src/synthorg/api/controllers/connections.py:66:22 src/synthorg/api/controllers/connections.py:156:29 src/synthorg/api/controllers/coordination_metrics.py:34:21 -src/synthorg/api/controllers/coordination_metrics.py:93:29 +src/synthorg/api/controllers/coordination_metrics.py:96:29 src/synthorg/api/controllers/custom_rules.py:205:29 src/synthorg/api/controllers/custom_rules.py:435:29 src/synthorg/api/controllers/departments.py:229:49 @@ -136,7 +135,7 @@ src/synthorg/api/services/idempotency_service.py:47:41 src/synthorg/api/services/idempotency_service.py:48:49 src/synthorg/api/services/org_mutations.py:43:22 src/synthorg/api/services/ssrf_violation_service.py:141:21 -src/synthorg/api/state_services.py:114:26 +src/synthorg/api/state_services.py:115:26 src/synthorg/backup/service_archive.py:38:23 src/synthorg/backup/service_archive.py:42:21 src/synthorg/budget/_optimizer_helpers.py:225:41 diff --git a/src/synthorg/api/controllers/activities.py b/src/synthorg/api/controllers/activities.py index 4c8f545483..34113c9790 100644 --- a/src/synthorg/api/controllers/activities.py +++ b/src/synthorg/api/controllers/activities.py @@ -36,58 +36,10 @@ API_ACTIVITY_FEED_QUERIED, API_REQUEST_ERROR, ) -from synthorg.settings.enums import SettingNamespace from synthorg.tools.invocation_record import ToolInvocationRecord # noqa: TC001 logger = get_logger(__name__) -# Fallback cap applied when no settings resolver is wired in. -_MAX_LIFECYCLE_EVENTS = 10_000 - -# Module-level log-once guard: during a prolonged settings outage -# this endpoint is queried once per request and would otherwise -# flood the logs with identical fallback warnings (plus traceback). -# The flag is set when we first emit the fallback warning and is -# cleared on the next successful resolution, so a later outage is -# visible again. -_lifecycle_cap_fallback_logged: bool = False - - -async def _resolve_lifecycle_cap(app_state: AppState) -> int: - """Resolve the active lifecycle-query cap, falling back to the constant. - - A settings outage or malformed value must not fail the endpoint; - the fallback constant keeps the DB-side ``LIMIT`` bounded even - when the resolver is unavailable. Warnings are log-once per run - of failures (cleared on recovery) to avoid flooding logs during - a prolonged outage; traceback logging is suppressed for the same - reason. - """ - global _lifecycle_cap_fallback_logged # noqa: PLW0603 - if not app_state.has_config_resolver: - return _MAX_LIFECYCLE_EVENTS - try: - value = await app_state.config_resolver.get_int( - SettingNamespace.API.value, "max_lifecycle_events_per_query" - ) - except asyncio.CancelledError: - raise - except MemoryError, RecursionError: - raise - except Exception as exc: - if not _lifecycle_cap_fallback_logged: - logger.warning( - API_REQUEST_ERROR, - note="failed to resolve max_lifecycle_events_per_query; using fallback", - error_type=type(exc).__name__, - error=safe_error_description(exc), - cap=_MAX_LIFECYCLE_EVENTS, - ) - _lifecycle_cap_fallback_logged = True - return _MAX_LIFECYCLE_EVENTS - _lifecycle_cap_fallback_logged = False - return value - # Degraded source names -- used in responses and tests. _SRC_PERFORMANCE_TRACKER = "performance_tracker" @@ -407,7 +359,7 @@ async def list_activities( # noqa: PLR0913 app_state: AppState = state.app_state now = datetime.now(UTC) since = now - timedelta(hours=last_n_hours) - lifecycle_cap = await _resolve_lifecycle_cap(app_state) + lifecycle_cap = app_state.api_bridge_config.max_lifecycle_events_per_query lifecycle_events = await app_state.persistence.lifecycle_events.list_events( agent_id=agent_id, diff --git a/src/synthorg/api/controllers/audit.py b/src/synthorg/api/controllers/audit.py index 2271e27539..51b49c1754 100644 --- a/src/synthorg/api/controllers/audit.py +++ b/src/synthorg/api/controllers/audit.py @@ -39,8 +39,11 @@ _MAX_AUDIT_QUERY = 10_000 """Fallback cap applied when no settings resolver is wired in.""" -# Module-level log-once guard for the settings-resolution fallback; -# see ``activities._resolve_lifecycle_cap`` for the rationale. +# Module-level log-once guard for the settings-resolution fallback: +# during a prolonged settings outage this endpoint is queried once per +# request and would otherwise flood the logs with identical warnings; +# the flag is set on first failure and cleared on the next successful +# resolution so a later outage is visible again. _audit_cap_fallback_logged: bool = False diff --git a/src/synthorg/api/controllers/coordination_metrics.py b/src/synthorg/api/controllers/coordination_metrics.py index fcce0fa36c..74f2506ae3 100644 --- a/src/synthorg/api/controllers/coordination_metrics.py +++ b/src/synthorg/api/controllers/coordination_metrics.py @@ -34,8 +34,11 @@ _MAX_METRICS_QUERY = 10_000 """Fallback cap applied when no settings resolver is wired in.""" -# Module-level log-once guard for the settings-resolution fallback; -# see ``activities._resolve_lifecycle_cap`` for the rationale. +# Module-level log-once guard for the settings-resolution fallback: +# during a prolonged settings outage this endpoint is queried once per +# request and would otherwise flood the logs with identical warnings; +# the flag is set on first failure and cleared on the next successful +# resolution so a later outage is visible again. _metrics_cap_fallback_logged: bool = False diff --git a/src/synthorg/api/lifecycle_helpers.py b/src/synthorg/api/lifecycle_helpers.py index 5b885d04ee..f01bfdeeae 100644 --- a/src/synthorg/api/lifecycle_helpers.py +++ b/src/synthorg/api/lifecycle_helpers.py @@ -15,6 +15,7 @@ API_APP_STARTUP, API_AUDIT_RETENTION, API_AUTH_LOCKOUT_CLEANUP, + API_BRIDGE_CONFIG_RESOLVE_FAILED, API_SESSION_CLEANUP, API_WS_TICKET_CLEANUP, ) @@ -30,6 +31,7 @@ registered_default_int, ) from synthorg.settings.subscribers import ( + ApiBridgeSettingsSubscriber, BackupSettingsSubscriber, MemorySettingsSubscriber, ObservabilitySettingsSubscriber, @@ -589,11 +591,16 @@ def _build_settings_dispatcher( app_state=app_state, settings_service=settings_service, ) + api_bridge_sub = ApiBridgeSettingsSubscriber( + app_state=app_state, + settings_service=settings_service, + ) subs: list[SettingsSubscriber] = [ provider_sub, memory_sub, observability_sub, per_op_rl_sub, + api_bridge_sub, ] if backup_service is not None: subs.append( @@ -754,6 +761,38 @@ async def _apply_notification_dispatcher_config( ) +async def _apply_api_bridge_config_snapshot(app_state: AppState) -> None: + """Snapshot ``ApiBridgeConfig`` onto ``AppState`` at startup. + + Resolves the full bridge once via + :meth:`ConfigResolver.get_api_bridge_config` and atomically swaps + it onto ``app_state``. On any non-fatal resolve failure the + default ``ApiBridgeConfig()`` snapshot installed by + ``AppState.__init__`` is retained and a single structured warning + is emitted -- the centralised replacement for the per-request + log-once fallback the activities controller used to carry inline. + + No-op when no resolver is wired (dev/test rigs that bypass + ``create_app``); the default snapshot remains in place. + """ + if not app_state.has_config_resolver: + return + try: + snapshot = await app_state.config_resolver.get_api_bridge_config() + except MemoryError, RecursionError: + raise + except Exception as exc: + logger.warning( + API_BRIDGE_CONFIG_RESOLVE_FAILED, + bridge="api", + error_type=type(exc).__name__, + error=safe_error_description(exc), + fallback="module_defaults", + ) + return + app_state.swap_api_bridge_config(snapshot) + + async def _apply_bridge_config( # noqa: C901, PLR0912, PLR0915 app_state: AppState, effective_config: RootConfig | None, @@ -768,6 +807,7 @@ async def _apply_bridge_config( # noqa: C901, PLR0912, PLR0915 return await _validate_approval_urgency_invariant(app_state) + await _apply_api_bridge_config_snapshot(app_state) try: app_state.ticket_store.set_max_pending_per_user( diff --git a/src/synthorg/api/state.py b/src/synthorg/api/state.py index b575c72300..45aa90bf99 100644 --- a/src/synthorg/api/state.py +++ b/src/synthorg/api/state.py @@ -108,6 +108,7 @@ from synthorg.security.audit import AuditLog # noqa: TC001 from synthorg.security.timeout.scheduler import ApprovalTimeoutScheduler # noqa: TC001 from synthorg.security.trust.service import TrustService # noqa: TC001 +from synthorg.settings.bridge_configs import ApiBridgeConfig from synthorg.settings.resolver import ConfigResolver from synthorg.settings.service import SettingsService # noqa: TC001 from synthorg.telemetry.collector import TelemetryCollector # noqa: TC001 @@ -155,6 +156,7 @@ class AppState(AppStateServicesMixin): "_agent_registry", "_agent_version_service", "_analytics_service", + "_api_bridge_config", "_approval_gate", "_approval_timeout_scheduler", "_artifact_facade_service", @@ -415,6 +417,15 @@ def __init__( # noqa: PLR0913, PLR0915 # built into the notification-dispatcher sinks rather than # rebuilding and closing them on every startup. self._bridge_config_applied: bool = False + # Frozen ``ApiBridgeConfig`` snapshot consumed by API + # controllers (e.g. activities lifecycle cap). Default- + # constructed so consumers always see a valid instance, even + # before ``_apply_bridge_config`` has run or when the resolver + # is unavailable; ``_apply_bridge_config`` swaps in the + # operator-tuned snapshot, and + # ``ApiBridgeSettingsSubscriber`` hot-swaps it on operator + # edits (no restart required). + self._api_bridge_config: ApiBridgeConfig = ApiBridgeConfig() self._provider_management: ProviderManagementService | None = None self._org_mutation_service: OrgMutationService | None = None # Shutdown flag observable by long-lived subsystems. diff --git a/src/synthorg/api/state_services.py b/src/synthorg/api/state_services.py index 1fd7f91af0..7c49144fd3 100644 --- a/src/synthorg/api/state_services.py +++ b/src/synthorg/api/state_services.py @@ -83,6 +83,7 @@ ) from synthorg.providers.registry import ProviderRegistry # noqa: TC001 from synthorg.providers.routing.router import ModelRouter # noqa: TC001 +from synthorg.settings.bridge_configs import ApiBridgeConfig # noqa: TC001 from synthorg.settings.resolver import ConfigResolver # noqa: TC001 from synthorg.settings.service import SettingsService # noqa: TC001 from synthorg.tools.invocation_tracker import ToolInvocationTracker # noqa: TC001 @@ -153,6 +154,7 @@ class AppStateServicesMixin(_FacadesMixin): _set_once: Any _init_derived_services: Any + _api_bridge_config: ApiBridgeConfig config: Any def _require_service[T]( # pragma: no cover @@ -1040,6 +1042,40 @@ def swap_per_op_concurrency_config( override_count=len(config.overrides), ) + @property + def api_bridge_config(self) -> ApiBridgeConfig: + """Return the current ``ApiBridgeConfig`` snapshot. + + Always non-None: ``__init__`` default-constructs an + ``ApiBridgeConfig()`` so consumers see valid defaults even + before ``_apply_bridge_config`` runs or when the resolver is + unreachable. Operator overrides land via + :meth:`swap_api_bridge_config` from the startup snapshot path + and the ``ApiBridgeSettingsSubscriber`` hot-reload path. + """ + return self._api_bridge_config + + def swap_api_bridge_config(self, config: ApiBridgeConfig) -> None: + """Replace the ``ApiBridgeConfig`` snapshot atomically. + + Called by ``_apply_bridge_config`` at startup with the value + resolved through ``ConfigResolver.get_api_bridge_config`` and by + :class:`ApiBridgeSettingsSubscriber` on operator-driven changes + to watched API settings. A plain Python attribute assignment + is atomic, so concurrent readers always see either the prior + snapshot or the new one -- never a half-built object. + """ + previous = self._api_bridge_config + self._api_bridge_config = config + if previous is config: + return + logger.info( + SETTINGS_SERVICE_SWAPPED, + service="api_bridge_config", + old_lifecycle_cap=previous.max_lifecycle_events_per_query, + new_lifecycle_cap=config.max_lifecycle_events_per_query, + ) + @property def has_backup_service(self) -> bool: """Check whether the backup service is configured.""" diff --git a/src/synthorg/settings/subscribers/__init__.py b/src/synthorg/settings/subscribers/__init__.py index cb68d51298..676e2f2f35 100644 --- a/src/synthorg/settings/subscribers/__init__.py +++ b/src/synthorg/settings/subscribers/__init__.py @@ -1,5 +1,8 @@ """Concrete settings change subscribers.""" +from synthorg.settings.subscribers.api_bridge_subscriber import ( + ApiBridgeSettingsSubscriber, +) from synthorg.settings.subscribers.backup_subscriber import ( BackupSettingsSubscriber, ) @@ -17,6 +20,7 @@ ) __all__ = [ + "ApiBridgeSettingsSubscriber", "BackupSettingsSubscriber", "MemorySettingsSubscriber", "ObservabilitySettingsSubscriber", diff --git a/src/synthorg/settings/subscribers/api_bridge_subscriber.py b/src/synthorg/settings/subscribers/api_bridge_subscriber.py new file mode 100644 index 0000000000..06e1ce1698 --- /dev/null +++ b/src/synthorg/settings/subscribers/api_bridge_subscriber.py @@ -0,0 +1,112 @@ +"""API bridge-config settings subscriber. + +Hot-swaps :attr:`AppState.api_bridge_config` when an operator edits a +watched ``api.*`` setting whose value lives on +:class:`~synthorg.settings.bridge_configs.ApiBridgeConfig`. The +dispatcher already filters out ``restart_required=True`` keys before +invoking subscribers, so the watched set here only enumerates the +hot-reloadable fields. + +Currently watches ``api.max_lifecycle_events_per_query``, consumed by +:class:`~synthorg.api.controllers.activities.ActivityController` as +its ``LIMIT`` clamp on the lifecycle-events query. Other +``ApiBridgeConfig`` fields can be added to ``_WATCHED`` as their +controllers migrate off per-request fallback constants. +""" + +from typing import TYPE_CHECKING + +from synthorg.observability import get_logger, safe_error_description +from synthorg.observability.events.settings import ( + SETTINGS_SERVICE_SWAP_FAILED, + SETTINGS_SUBSCRIBER_NOTIFIED, +) + +if TYPE_CHECKING: + from synthorg.api.state import AppState + from synthorg.settings.service import SettingsService + +logger = get_logger(__name__) + +_NAMESPACE = "api" +_WATCHED: frozenset[tuple[str, str]] = frozenset( + { + (_NAMESPACE, "max_lifecycle_events_per_query"), + } +) + + +class ApiBridgeSettingsSubscriber: + """Hot-swap ``api_bridge_config`` when watched API settings change. + + Holds references to :class:`AppState` (where the snapshot lives) + and :class:`SettingsService` (carried for parity with peer + subscribers; the resolver is reached via ``app_state.config_resolver`` + so the subscriber sees the same DB > env > YAML > default chain + every other consumer does). + + On a watched-key change the subscriber resolves the integer value + via :class:`~synthorg.settings.resolver.ConfigResolver`, builds the + next snapshot through ``model_copy(update=...)`` (preserves every + other field), and hands it to ``AppState.swap_api_bridge_config``. + Resolver failures are logged via ``SETTINGS_SERVICE_SWAP_FAILED`` + and re-raised so the dispatcher records subscriber context; the + previous snapshot stays in place. + + Args: + app_state: Application state that owns the live snapshot. + settings_service: Settings service held for symmetry with peer + subscribers. + """ + + def __init__( + self, + app_state: AppState, + settings_service: SettingsService, + ) -> None: + self._app_state = app_state + self._settings_service = settings_service + + @property + def watched_keys(self) -> frozenset[tuple[str, str]]: + """Return the ``(namespace, key)`` pairs this subscriber watches.""" + return _WATCHED + + @property + def subscriber_name(self) -> str: + """Human-readable subscriber name for logs.""" + return "api-bridge-config" + + async def on_settings_changed( + self, + namespace: str, + key: str, + ) -> None: + """Resolve the new value and swap the bridge-config snapshot.""" + if (namespace, key) not in _WATCHED: + logger.warning( + SETTINGS_SUBSCRIBER_NOTIFIED, + subscriber=self.subscriber_name, + namespace=namespace, + key=key, + note="ignored unexpected pair", + ) + return + try: + value = await self._app_state.config_resolver.get_int(namespace, key) + new_config = self._app_state.api_bridge_config.model_copy( + update={key: value}, + ) + except MemoryError, RecursionError: + raise + except Exception as exc: + logger.warning( + SETTINGS_SERVICE_SWAP_FAILED, + service="api_bridge_config", + trigger_namespace=namespace, + trigger_key=key, + error_type=type(exc).__name__, + error=safe_error_description(exc), + ) + raise + self._app_state.swap_api_bridge_config(new_config) diff --git a/tests/unit/api/controllers/test_activities.py b/tests/unit/api/controllers/test_activities.py index 300c0882fb..e755a5ab02 100644 --- a/tests/unit/api/controllers/test_activities.py +++ b/tests/unit/api/controllers/test_activities.py @@ -714,3 +714,85 @@ async def test_degraded_budget_config( assert "budget_config" in resp.json()["degraded_sources"] finally: app_state.config_resolver.get_budget_config = original + + +class TestActivityFeedLifecycleCap: + """Lifecycle cap is sourced from ``app_state.api_bridge_config``. + + The controller no longer carries a hardcoded fallback constant; + the cap flows through the bridge-config snapshot that + ``_apply_bridge_config`` populates at startup and the + :class:`ApiBridgeSettingsSubscriber` hot-swaps on operator change. + """ + + async def test_default_cap_passed_to_list_events( + self, + test_client: TestClient[Any], + fake_persistence: FakePersistenceBackend, + ) -> None: + captured: dict[str, Any] = {} + original = fake_persistence.lifecycle_events.list_events + + async def _spy(**kwargs: Any) -> Any: + captured.update(kwargs) + return await original(**kwargs) + + fake_persistence.lifecycle_events.list_events = _spy + try: + resp = test_client.get("/api/v1/activities") + finally: + fake_persistence.lifecycle_events.list_events = original + + assert resp.status_code == 200 + app_state = test_client.app.state.app_state + assert ( + captured["limit"] + == app_state.api_bridge_config.max_lifecycle_events_per_query + ) + + async def test_swapped_cap_takes_effect_immediately( + self, + test_client: TestClient[Any], + fake_persistence: FakePersistenceBackend, + ) -> None: + from synthorg.settings.bridge_configs import ApiBridgeConfig + + app_state = test_client.app.state.app_state + previous = app_state.api_bridge_config + app_state.swap_api_bridge_config( + previous.model_copy(update={"max_lifecycle_events_per_query": 137}), + ) + captured: dict[str, Any] = {} + original = fake_persistence.lifecycle_events.list_events + + async def _spy(**kwargs: Any) -> Any: + captured.update(kwargs) + return await original(**kwargs) + + fake_persistence.lifecycle_events.list_events = _spy + try: + resp = test_client.get("/api/v1/activities") + finally: + fake_persistence.lifecycle_events.list_events = original + app_state.swap_api_bridge_config(previous) + + assert resp.status_code == 200 + assert captured["limit"] == 137 + # Confirm the snapshot still holds an ``ApiBridgeConfig`` after + # restoration -- the swap path is reversible. + assert isinstance(app_state.api_bridge_config, ApiBridgeConfig) + + +class TestActivitiesControllerSurface: + """The legacy fallback symbols are gone after the refactor.""" + + def test_max_lifecycle_events_constant_removed(self) -> None: + from synthorg.api.controllers import activities as mod + + assert not hasattr(mod, "_MAX_LIFECYCLE_EVENTS") + + def test_resolve_lifecycle_cap_helper_removed(self) -> None: + from synthorg.api.controllers import activities as mod + + assert not hasattr(mod, "_resolve_lifecycle_cap") + assert not hasattr(mod, "_lifecycle_cap_fallback_logged") diff --git a/tests/unit/api/test_apply_api_bridge_config_snapshot.py b/tests/unit/api/test_apply_api_bridge_config_snapshot.py new file mode 100644 index 0000000000..5074a6de76 --- /dev/null +++ b/tests/unit/api/test_apply_api_bridge_config_snapshot.py @@ -0,0 +1,103 @@ +"""Tests for the ``_apply_api_bridge_config_snapshot`` startup helper. + +The activities controller (and any future consumer) reads +``app_state.api_bridge_config.`` for operator-tunable knobs. +At startup the helper resolves the full ``ApiBridgeConfig`` from +``ConfigResolver`` and atomically swaps it onto ``AppState``. On +failure the default snapshot is retained and a single structured +warning is emitted -- the centralised replacement for the per-request +log-once fallback the activities controller used to carry inline. +""" + +from typing import Any, cast +from unittest.mock import create_autospec + +import pytest + +from synthorg.api.approval_store import ApprovalStore +from synthorg.api.lifecycle_helpers import _apply_api_bridge_config_snapshot +from synthorg.api.state import AppState +from synthorg.config.schema import RootConfig +from synthorg.settings.bridge_configs import ApiBridgeConfig +from synthorg.settings.resolver import ConfigResolver + +pytestmark = pytest.mark.unit + + +def _make_state(*, config_resolver: ConfigResolver | None) -> AppState: + state = AppState( + config=RootConfig(company_name="test"), + approval_store=ApprovalStore(), + ) + state._config_resolver = config_resolver + return state + + +def _resolver_returning(snapshot: ApiBridgeConfig) -> ConfigResolver: + resolver = create_autospec(ConfigResolver, instance=True) + resolver.get_api_bridge_config.return_value = snapshot + return cast("ConfigResolver", resolver) + + +def _resolver_raising(exc: BaseException) -> ConfigResolver: + resolver = create_autospec(ConfigResolver, instance=True) + resolver.get_api_bridge_config.side_effect = exc + return cast("ConfigResolver", resolver) + + +class TestApplyApiBridgeConfigSnapshot: + """Startup snapshot wiring for ApiBridgeConfig.""" + + async def test_no_resolver_keeps_default_snapshot(self) -> None: + state = _make_state(config_resolver=None) + await _apply_api_bridge_config_snapshot(state) + assert state.api_bridge_config == ApiBridgeConfig() + + async def test_happy_path_swaps_snapshot(self) -> None: + custom = ApiBridgeConfig(max_lifecycle_events_per_query=25_000) + state = _make_state(config_resolver=_resolver_returning(custom)) + + await _apply_api_bridge_config_snapshot(state) + + assert state.api_bridge_config is custom + assert state.api_bridge_config.max_lifecycle_events_per_query == 25_000 + + async def test_failure_keeps_default_and_logs_warning( + self, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + state = _make_state( + config_resolver=_resolver_raising(RuntimeError("resolver outage")), + ) + + warnings: list[tuple[str, dict[str, Any]]] = [] + + from synthorg.api import lifecycle_helpers as mod + + original = mod.logger.warning + + def _capture(event: str, **kwargs: Any) -> None: + warnings.append((event, kwargs)) + original(event, **kwargs) + + monkeypatch.setattr(mod.logger, "warning", _capture) + + await _apply_api_bridge_config_snapshot(state) + + assert state.api_bridge_config == ApiBridgeConfig() + # Single warning emitted with the canonical event name and the + # redacted error description -- never the raw exception string. + assert len(warnings) == 1 + event_name, fields = warnings[0] + assert event_name == "api.bridge_config.resolve_failed" + assert fields["bridge"] == "api" + assert fields["error_type"] == "RuntimeError" + # ``safe_error_description`` redacts message bodies; we just + # confirm the field is present (not asserting on its value). + assert "error" in fields + + async def test_memory_error_propagates(self) -> None: + state = _make_state(config_resolver=_resolver_raising(MemoryError())) + + with pytest.raises(MemoryError): + await _apply_api_bridge_config_snapshot(state) diff --git a/tests/unit/api/test_state.py b/tests/unit/api/test_state.py index 3e50c1f259..a8389b9ace 100644 --- a/tests/unit/api/test_state.py +++ b/tests/unit/api/test_state.py @@ -525,3 +525,43 @@ async def test_eviction_preserves_referenced_lock( assert state._request_locks["in-flight"] is reserved finally: state._release_request_lock_ref("in-flight") + + +@pytest.mark.unit +class TestAppStateApiBridgeConfig: + """Tests for api_bridge_config snapshot accessor and swap_api_bridge_config.""" + + def test_default_snapshot_available_pre_startup(self) -> None: + from synthorg.settings.bridge_configs import ApiBridgeConfig + + state = _make_state() + snapshot = state.api_bridge_config + assert isinstance(snapshot, ApiBridgeConfig) + assert snapshot == ApiBridgeConfig() + + def test_default_lifecycle_cap_matches_bridge_default(self) -> None: + from synthorg.settings.bridge_configs import ApiBridgeConfig + + state = _make_state() + assert ( + state.api_bridge_config.max_lifecycle_events_per_query + == ApiBridgeConfig().max_lifecycle_events_per_query + ) + + def test_swap_replaces_snapshot(self) -> None: + from synthorg.settings.bridge_configs import ApiBridgeConfig + + state = _make_state() + new = ApiBridgeConfig(max_lifecycle_events_per_query=25_000) + state.swap_api_bridge_config(new) + assert state.api_bridge_config is new + assert state.api_bridge_config.max_lifecycle_events_per_query == 25_000 + + def test_swap_is_idempotent_for_same_instance(self) -> None: + from synthorg.settings.bridge_configs import ApiBridgeConfig + + state = _make_state() + snapshot = ApiBridgeConfig(max_lifecycle_events_per_query=12_345) + state.swap_api_bridge_config(snapshot) + state.swap_api_bridge_config(snapshot) + assert state.api_bridge_config is snapshot diff --git a/tests/unit/settings/test_api_bridge_subscriber.py b/tests/unit/settings/test_api_bridge_subscriber.py new file mode 100644 index 0000000000..ba26493373 --- /dev/null +++ b/tests/unit/settings/test_api_bridge_subscriber.py @@ -0,0 +1,150 @@ +"""Tests for ``ApiBridgeSettingsSubscriber``. + +The subscriber hot-swaps ``app_state.api_bridge_config`` when the +operator changes a watched API setting. Tests cover protocol +conformance, happy-path swap (single field updated via +``model_copy``, every other field preserved), unexpected key/namespace +no-op, and resolver-failure path (no swap, error re-raised so the +dispatcher logs subscriber context). +""" + +from unittest.mock import create_autospec + +import pytest + +from synthorg.api.approval_store import ApprovalStore +from synthorg.api.state import AppState +from synthorg.config.schema import RootConfig +from synthorg.settings.bridge_configs import ApiBridgeConfig +from synthorg.settings.resolver import ConfigResolver +from synthorg.settings.service import SettingsService +from synthorg.settings.subscriber import SettingsSubscriber +from synthorg.settings.subscribers.api_bridge_subscriber import ( + ApiBridgeSettingsSubscriber, +) + +pytestmark = pytest.mark.unit + + +def _make_subscriber( + *, + snapshot: ApiBridgeConfig | None = None, + resolver_int_return: int | None = None, + resolver_int_side_effect: BaseException | None = None, +) -> tuple[ApiBridgeSettingsSubscriber, AppState]: + """Build a subscriber with a real AppState + spec'd ConfigResolver. + + Returns the subscriber plus the AppState so callers can assert + on the post-call ``api_bridge_config`` snapshot. + """ + settings_service = create_autospec(SettingsService, instance=True) + + resolver = create_autospec(ConfigResolver, instance=True) + if resolver_int_side_effect is not None: + resolver.get_int.side_effect = resolver_int_side_effect + else: + resolver.get_int.return_value = resolver_int_return + + app_state = AppState( + config=RootConfig(company_name="test"), + approval_store=ApprovalStore(), + ) + app_state._config_resolver = resolver + if snapshot is not None: + app_state.swap_api_bridge_config(snapshot) + + sub = ApiBridgeSettingsSubscriber( + app_state=app_state, + settings_service=settings_service, + ) + return sub, app_state + + +class TestSubscriberProtocol: + """``ApiBridgeSettingsSubscriber`` conforms to ``SettingsSubscriber``.""" + + def test_isinstance_check(self) -> None: + sub, _ = _make_subscriber(resolver_int_return=10_000) + assert isinstance(sub, SettingsSubscriber) + + def test_watched_keys(self) -> None: + sub, _ = _make_subscriber(resolver_int_return=10_000) + assert sub.watched_keys == frozenset( + {("api", "max_lifecycle_events_per_query")} + ) + + def test_subscriber_name(self) -> None: + sub, _ = _make_subscriber(resolver_int_return=10_000) + assert sub.subscriber_name == "api-bridge-config" + + +class TestRebuild: + """``on_settings_changed`` rebuilds + swaps the snapshot.""" + + async def test_lifecycle_cap_change_swaps_with_model_copy(self) -> None: + original = ApiBridgeConfig( + max_lifecycle_events_per_query=10_000, + max_audit_records_per_query=42_000, + ) + sub, app_state = _make_subscriber( + snapshot=original, + resolver_int_return=50_000, + ) + + await sub.on_settings_changed("api", "max_lifecycle_events_per_query") + + swapped = app_state.api_bridge_config + assert swapped.max_lifecycle_events_per_query == 50_000 + # Every other field is preserved verbatim from the prior snapshot. + assert swapped.max_audit_records_per_query == 42_000 + + async def test_resolver_failure_does_not_swap(self) -> None: + original = ApiBridgeConfig(max_lifecycle_events_per_query=8_000) + sub, app_state = _make_subscriber( + snapshot=original, + resolver_int_side_effect=RuntimeError("resolver outage"), + ) + + with pytest.raises(RuntimeError, match="resolver outage"): + await sub.on_settings_changed("api", "max_lifecycle_events_per_query") + + # Snapshot retained from before the change. + assert app_state.api_bridge_config is original + assert app_state.api_bridge_config.max_lifecycle_events_per_query == 8_000 + + async def test_memory_error_propagates(self) -> None: + sub, app_state = _make_subscriber( + resolver_int_side_effect=MemoryError(), + ) + before = app_state.api_bridge_config + + with pytest.raises(MemoryError): + await sub.on_settings_changed("api", "max_lifecycle_events_per_query") + + assert app_state.api_bridge_config is before + + +class TestUnexpectedRouting: + """Unexpected (namespace, key) pairs are logged and no-op.""" + + async def test_unknown_namespace_is_ignored(self) -> None: + original = ApiBridgeConfig(max_lifecycle_events_per_query=4_321) + sub, app_state = _make_subscriber( + snapshot=original, + resolver_int_return=99_999, + ) + + await sub.on_settings_changed("other", "max_lifecycle_events_per_query") + + assert app_state.api_bridge_config is original + + async def test_unknown_key_is_ignored(self) -> None: + original = ApiBridgeConfig(max_lifecycle_events_per_query=4_321) + sub, app_state = _make_subscriber( + snapshot=original, + resolver_int_return=99_999, + ) + + await sub.on_settings_changed("api", "some_unrelated_key") + + assert app_state.api_bridge_config is original From 754da41c3d0ca1474505f3d8bbd77b913212bbdf Mon Sep 17 00:00:00 2001 From: Aurelio <19254254+Aureliolo@users.noreply.github.com> Date: Sat, 9 May 2026 13:49:57 +0200 Subject: [PATCH 2/5] =?UTF-8?q?refactor(api):=20triage=20fixes=20=E2=80=94?= =?UTF-8?q?=20CancelledError=20guard,=20lock-guarded=20mutate=5Fapi=5Fbrid?= =?UTF-8?q?ge=5Fconfig,=20watched-key=20startup=20check,=20validation-on-m?= =?UTF-8?q?utate,=20snapshot=20pattern=20docs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/reference/configuration-precedence.md | 46 +++++++++++++ src/synthorg/api/controllers/audit.py | 6 +- .../api/controllers/coordination_metrics.py | 6 +- src/synthorg/api/lifecycle_helpers.py | 2 + src/synthorg/api/state.py | 9 ++- src/synthorg/api/state_services.py | 68 +++++++++++++++---- .../subscribers/api_bridge_subscriber.py | 21 ++++-- .../settings/test_api_bridge_subscriber.py | 21 ++++++ 8 files changed, 159 insertions(+), 20 deletions(-) diff --git a/docs/reference/configuration-precedence.md b/docs/reference/configuration-precedence.md index 4ce2b7922b..959eb40345 100644 --- a/docs/reference/configuration-precedence.md +++ b/docs/reference/configuration-precedence.md @@ -141,6 +141,52 @@ entry: `tests/unit/settings/test_precedence_chain.py`. 4. Document the new entry in this page's source matrix. +## Bridge-config snapshot pattern (hot-reloadable AppState fields) + +For controller / service knobs that should be hot-reloadable but cost +too much to resolve through `ConfigResolver.get_*()` on every request, +the canonical pattern is a frozen Pydantic snapshot on `AppState` +populated at startup and hot-swapped by a settings subscriber on +operator-driven changes. Reference implementation: +`api.max_lifecycle_events_per_query` consumed by +`ActivityController.list_activities`. + +The pattern has four pieces: + +1. **Frozen bridge model.** A class in + `synthorg/settings/bridge_configs.py` (e.g. `ApiBridgeConfig`) with + `model_config = ConfigDict(frozen=True, allow_inf_nan=False, extra="forbid")`, + one field per setting it carries, defaults that match the + registered defaults. The model is the single source of truth for + the fallback value -- no controller carries a duplicate constant. +2. **Resolver builder.** `ConfigResolver.get__bridge_config()` + resolves every field at once via `_resolve_bridge_fields()`. +3. **AppState slot + accessors.** `AppState.__init__` default- + constructs the bridge model so consumers always see a valid + snapshot, even before `_apply_bridge_config` has run. + `AppState._bridge_config` returns the current snapshot; + `AppState.swap__bridge_config(config)` does a wholesale + replace under a per-bridge `threading.Lock`; + `AppState.mutate__bridge_config({field: value, ...})` + applies a partial update under the same lock so two concurrent + subscribers cannot lose each other's writes. +4. **Settings subscriber.** A `SettingsSubscriber` implementation in + `synthorg/settings/subscribers/_bridge_subscriber.py` whose + `_WATCHED` set lists every hot-reloadable field. On change, the + subscriber resolves the new value and calls `mutate_*` with the + single-field update; `model_copy(update=...)` re-validates against + the field's `Field(ge=..., le=...)` bounds, so an out-of-range + value raises `ValidationError` and the prior snapshot is retained. + Module-load-time guard: every key in `_WATCHED` is asserted to + exist on the bridge model so a typo or rename surfaces at import, + not on the next operator hot-reload. + +Use this pattern when the setting is hot-reloadable +(`restart_required=False`) but per-request resolver lookup would +add overhead or coupling. For restart-required knobs (e.g. +`ws_auth_timeout_seconds`) the simpler `set_*()` pattern in +`_apply_bridge_config` is sufficient. + ## Bootstrap-wiring trace (ghost-wired settings gate) A registered setting whose consuming machinery exists but is never diff --git a/src/synthorg/api/controllers/audit.py b/src/synthorg/api/controllers/audit.py index 51b49c1754..f48b64be64 100644 --- a/src/synthorg/api/controllers/audit.py +++ b/src/synthorg/api/controllers/audit.py @@ -43,7 +43,11 @@ # during a prolonged settings outage this endpoint is queried once per # request and would otherwise flood the logs with identical warnings; # the flag is set on first failure and cleared on the next successful -# resolution so a later outage is visible again. +# resolution so a later outage is visible again. The activities +# controller has migrated this knob to ``app_state.api_bridge_config`` +# (startup snapshot + ``ApiBridgeSettingsSubscriber`` hot-reload); this +# controller follows the same pattern when its cap is similarly +# bridged. _audit_cap_fallback_logged: bool = False diff --git a/src/synthorg/api/controllers/coordination_metrics.py b/src/synthorg/api/controllers/coordination_metrics.py index 74f2506ae3..62e86ba220 100644 --- a/src/synthorg/api/controllers/coordination_metrics.py +++ b/src/synthorg/api/controllers/coordination_metrics.py @@ -38,7 +38,11 @@ # during a prolonged settings outage this endpoint is queried once per # request and would otherwise flood the logs with identical warnings; # the flag is set on first failure and cleared on the next successful -# resolution so a later outage is visible again. +# resolution so a later outage is visible again. The activities +# controller has migrated this knob to ``app_state.api_bridge_config`` +# (startup snapshot + ``ApiBridgeSettingsSubscriber`` hot-reload); this +# controller follows the same pattern when its cap is similarly +# bridged. _metrics_cap_fallback_logged: bool = False diff --git a/src/synthorg/api/lifecycle_helpers.py b/src/synthorg/api/lifecycle_helpers.py index f01bfdeeae..cc3571c572 100644 --- a/src/synthorg/api/lifecycle_helpers.py +++ b/src/synthorg/api/lifecycle_helpers.py @@ -779,6 +779,8 @@ async def _apply_api_bridge_config_snapshot(app_state: AppState) -> None: return try: snapshot = await app_state.config_resolver.get_api_bridge_config() + except asyncio.CancelledError: + raise except MemoryError, RecursionError: raise except Exception as exc: diff --git a/src/synthorg/api/state.py b/src/synthorg/api/state.py index 45aa90bf99..6f6156b145 100644 --- a/src/synthorg/api/state.py +++ b/src/synthorg/api/state.py @@ -157,6 +157,7 @@ class AppState(AppStateServicesMixin): "_agent_version_service", "_analytics_service", "_api_bridge_config", + "_api_bridge_config_lock", "_approval_gate", "_approval_timeout_scheduler", "_artifact_facade_service", @@ -424,8 +425,14 @@ def __init__( # noqa: PLR0913, PLR0915 # is unavailable; ``_apply_bridge_config`` swaps in the # operator-tuned snapshot, and # ``ApiBridgeSettingsSubscriber`` hot-swaps it on operator - # edits (no restart required). + # edits (no restart required). The dedicated lock guards the + # read-modify-write path on ``mutate_api_bridge_config`` so two + # concurrent subscribers each computing + # ``model_copy(update=...)`` from the same prior snapshot + # cannot lose each other's updates by both calling + # ``swap_api_bridge_config`` based on a stale read. self._api_bridge_config: ApiBridgeConfig = ApiBridgeConfig() + self._api_bridge_config_lock: threading.Lock = threading.Lock() self._provider_management: ProviderManagementService | None = None self._org_mutation_service: OrgMutationService | None = None # Shutdown flag observable by long-lived subsystems. diff --git a/src/synthorg/api/state_services.py b/src/synthorg/api/state_services.py index 7c49144fd3..dc4bcea551 100644 --- a/src/synthorg/api/state_services.py +++ b/src/synthorg/api/state_services.py @@ -155,6 +155,7 @@ class AppStateServicesMixin(_FacadesMixin): _set_once: Any _init_derived_services: Any _api_bridge_config: ApiBridgeConfig + _api_bridge_config_lock: threading.Lock config: Any def _require_service[T]( # pragma: no cover @@ -1051,29 +1052,70 @@ def api_bridge_config(self) -> ApiBridgeConfig: before ``_apply_bridge_config`` runs or when the resolver is unreachable. Operator overrides land via :meth:`swap_api_bridge_config` from the startup snapshot path - and the ``ApiBridgeSettingsSubscriber`` hot-reload path. + and :meth:`mutate_api_bridge_config` from the + ``ApiBridgeSettingsSubscriber`` hot-reload path. """ return self._api_bridge_config def swap_api_bridge_config(self, config: ApiBridgeConfig) -> None: - """Replace the ``ApiBridgeConfig`` snapshot atomically. - - Called by ``_apply_bridge_config`` at startup with the value - resolved through ``ConfigResolver.get_api_bridge_config`` and by - :class:`ApiBridgeSettingsSubscriber` on operator-driven changes - to watched API settings. A plain Python attribute assignment - is atomic, so concurrent readers always see either the prior - snapshot or the new one -- never a half-built object. + """Replace the ``ApiBridgeConfig`` snapshot wholesale. + + Used by ``_apply_bridge_config`` at startup with the value + resolved through ``ConfigResolver.get_api_bridge_config`` (full + snapshot, not a diff). Hot-reload paths must use + :meth:`mutate_api_bridge_config` instead so the read-modify- + write is serialised against concurrent updates. + + Acquires ``_api_bridge_config_lock`` so a concurrent + ``mutate_api_bridge_config`` cannot interleave its read with + this assignment and lose the partial update. """ - previous = self._api_bridge_config - self._api_bridge_config = config + with self._api_bridge_config_lock: + previous = self._api_bridge_config + self._api_bridge_config = config if previous is config: return + prev_fields = previous.model_dump() + new_fields = config.model_dump() + changed = sorted(k for k in new_fields if prev_fields.get(k) != new_fields[k]) + logger.info( + SETTINGS_SERVICE_SWAPPED, + service="api_bridge_config", + transition="swap", + changed_fields=changed, + ) + + def mutate_api_bridge_config(self, updates: dict[str, object]) -> None: + """Apply ``updates`` to the current snapshot under a lock. + + Combines a re-validating partial update and the swap into a + single critical section so two concurrent operator edits cannot + both build a new snapshot from the same prior value and lose + each other's update. The watched-key check in + :class:`~synthorg.settings.subscribers.api_bridge_subscriber.ApiBridgeSettingsSubscriber` + already restricts ``updates`` to fields declared on + ``ApiBridgeConfig``. + + Re-validation is forced via ``model_validate()`` rather + than ``model_copy(update=...)`` because Pydantic v2 skips + validators on the bare ``update=`` path -- an out-of-range + operator-supplied value (e.g. ``50`` against + ``Field(ge=100, le=1_000_000)``) would otherwise land silently + in the snapshot. Re-validation raises ``ValidationError``, + leaving the prior snapshot in place and propagating the failure + to the subscriber's error log. + """ + with self._api_bridge_config_lock: + previous = self._api_bridge_config + merged = previous.model_dump() + merged.update(updates) + new_config = type(previous).model_validate(merged) + self._api_bridge_config = new_config logger.info( SETTINGS_SERVICE_SWAPPED, service="api_bridge_config", - old_lifecycle_cap=previous.max_lifecycle_events_per_query, - new_lifecycle_cap=config.max_lifecycle_events_per_query, + transition="mutate", + changed_fields=sorted(updates), ) @property diff --git a/src/synthorg/settings/subscribers/api_bridge_subscriber.py b/src/synthorg/settings/subscribers/api_bridge_subscriber.py index 06e1ce1698..17e21547be 100644 --- a/src/synthorg/settings/subscribers/api_bridge_subscriber.py +++ b/src/synthorg/settings/subscribers/api_bridge_subscriber.py @@ -21,6 +21,7 @@ SETTINGS_SERVICE_SWAP_FAILED, SETTINGS_SUBSCRIBER_NOTIFIED, ) +from synthorg.settings.bridge_configs import ApiBridgeConfig if TYPE_CHECKING: from synthorg.api.state import AppState @@ -35,6 +36,21 @@ } ) +# Surface a typo or rename in ``_WATCHED`` at import time rather than at +# the next operator hot-reload. ``model_copy(update={key: value})`` would +# raise ValidationError because ``ApiBridgeConfig`` has ``extra="forbid"``, +# but only when the subscriber actually fires, so a deployment can ship +# with a broken watch list and never notice until a customer edits the +# offending setting. +_API_BRIDGE_FIELDS: frozenset[str] = frozenset(ApiBridgeConfig.model_fields) +for _ns, _key in _WATCHED: + if _key not in _API_BRIDGE_FIELDS: + msg = ( + f"ApiBridgeSettingsSubscriber._WATCHED key {_key!r}" + f" is not a field of ApiBridgeConfig" + ) + raise RuntimeError(msg) + class ApiBridgeSettingsSubscriber: """Hot-swap ``api_bridge_config`` when watched API settings change. @@ -94,9 +110,7 @@ async def on_settings_changed( return try: value = await self._app_state.config_resolver.get_int(namespace, key) - new_config = self._app_state.api_bridge_config.model_copy( - update={key: value}, - ) + self._app_state.mutate_api_bridge_config({key: value}) except MemoryError, RecursionError: raise except Exception as exc: @@ -109,4 +123,3 @@ async def on_settings_changed( error=safe_error_description(exc), ) raise - self._app_state.swap_api_bridge_config(new_config) diff --git a/tests/unit/settings/test_api_bridge_subscriber.py b/tests/unit/settings/test_api_bridge_subscriber.py index ba26493373..0da545a964 100644 --- a/tests/unit/settings/test_api_bridge_subscriber.py +++ b/tests/unit/settings/test_api_bridge_subscriber.py @@ -123,6 +123,27 @@ async def test_memory_error_propagates(self) -> None: assert app_state.api_bridge_config is before + async def test_out_of_range_value_rejected_keeps_prior_snapshot(self) -> None: + # ``ApiBridgeConfig.max_lifecycle_events_per_query`` is bounded by + # ``Field(ge=100, le=1_000_000)``. ``model_copy(update=...)`` re- + # validates, so an operator-supplied value below 100 must raise + # ``ValidationError`` -- the subscriber logs + re-raises and the + # prior snapshot stays in place. + original = ApiBridgeConfig(max_lifecycle_events_per_query=12_345) + sub, app_state = _make_subscriber( + snapshot=original, + resolver_int_return=50, # below the ge=100 bound + ) + + with pytest.raises(Exception) as exc_info: # noqa: PT011 -- pydantic ValidationError + await sub.on_settings_changed("api", "max_lifecycle_events_per_query") + assert exc_info.type.__name__ == "ValidationError" + + # Prior snapshot retained because the swap never happens when + # validation fails. + assert app_state.api_bridge_config is original + assert app_state.api_bridge_config.max_lifecycle_events_per_query == 12_345 + class TestUnexpectedRouting: """Unexpected (namespace, key) pairs are logged and no-op.""" From 7a9a6c575a52d9a260ec12ef50515d824d99d2cd Mon Sep 17 00:00:00 2001 From: Aurelio <19254254+Aureliolo@users.noreply.github.com> Date: Sat, 9 May 2026 14:03:00 +0200 Subject: [PATCH 3/5] chore: bump no_magic_numbers_baseline line numbers after comment additions --- scripts/no_magic_numbers_baseline.txt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/no_magic_numbers_baseline.txt b/scripts/no_magic_numbers_baseline.txt index 2c9a01a1b0..e99ff32a03 100644 --- a/scripts/no_magic_numbers_baseline.txt +++ b/scripts/no_magic_numbers_baseline.txt @@ -44,7 +44,7 @@ src/synthorg/api/controllers/approvals.py:75:40 src/synthorg/api/controllers/approvals.py:519:29 src/synthorg/api/controllers/artifacts.py:181:29 src/synthorg/api/controllers/audit.py:39:19 -src/synthorg/api/controllers/audit.py:104:29 +src/synthorg/api/controllers/audit.py:108:29 src/synthorg/api/controllers/backup.py:187:29 src/synthorg/api/controllers/budget.py:302:29 src/synthorg/api/controllers/budget_config_versions.py:46:29 @@ -54,7 +54,7 @@ src/synthorg/api/controllers/connections.py:65:20 src/synthorg/api/controllers/connections.py:66:22 src/synthorg/api/controllers/connections.py:156:29 src/synthorg/api/controllers/coordination_metrics.py:34:21 -src/synthorg/api/controllers/coordination_metrics.py:96:29 +src/synthorg/api/controllers/coordination_metrics.py:100:29 src/synthorg/api/controllers/custom_rules.py:205:29 src/synthorg/api/controllers/custom_rules.py:435:29 src/synthorg/api/controllers/departments.py:229:49 From aa9c12ff9f57d7d771a12c17fbb8d356b026f71e Mon Sep 17 00:00:00 2001 From: Aurelio <19254254+Aureliolo@users.noreply.github.com> Date: Sat, 9 May 2026 15:00:14 +0200 Subject: [PATCH 4/5] fix: babysit round 1, 5 findings (5 coderabbit, 0 gemini, 0 ci) - docs/reference/configuration-precedence.md: clarify mutate_* re-validates via model_validate (Pydantic v2 skips validators on bare model_copy(update=...)) - src/synthorg/api/controllers/audit.py: drop migration framing from log-once guard comment - src/synthorg/api/controllers/coordination_metrics.py: drop migration framing from log-once guard comment - src/synthorg/settings/subscribers/api_bridge_subscriber.py: drop 'as controllers migrate' framing from module docstring; sync class docstring with mutate_api_bridge_config (was claiming model_copy/swap) Skipped (factually wrong): - gemini lifecycle_helpers.py:785 / api_bridge_subscriber.py:115 unparenthesised except: PEP 758 form is mandated by CLAUDE.md and CodeRabbit baseline learning (Py3.14+ repo) - gemini api_bridge_subscriber.py:112 type-aware get_int: _WATCHED only carries one int field; type-dispatch is YAGNI scaffolding without a non-int consumer --- docs/reference/configuration-precedence.md | 8 ++++--- src/synthorg/api/controllers/audit.py | 10 +++------ .../api/controllers/coordination_metrics.py | 10 +++------ .../subscribers/api_bridge_subscriber.py | 22 ++++++++++--------- 4 files changed, 23 insertions(+), 27 deletions(-) diff --git a/docs/reference/configuration-precedence.md b/docs/reference/configuration-precedence.md index 959eb40345..5ef9ae57a0 100644 --- a/docs/reference/configuration-precedence.md +++ b/docs/reference/configuration-precedence.md @@ -174,9 +174,11 @@ The pattern has four pieces: `synthorg/settings/subscribers/_bridge_subscriber.py` whose `_WATCHED` set lists every hot-reloadable field. On change, the subscriber resolves the new value and calls `mutate_*` with the - single-field update; `model_copy(update=...)` re-validates against - the field's `Field(ge=..., le=...)` bounds, so an out-of-range - value raises `ValidationError` and the prior snapshot is retained. + single-field update; `mutate_*` re-validates the merged dict via + `model_validate(...)` (Pydantic v2 skips validators on the bare + `model_copy(update=...)` path) against the field's + `Field(ge=..., le=...)` bounds, so an out-of-range value raises + `ValidationError` and the prior snapshot is retained. Module-load-time guard: every key in `_WATCHED` is asserted to exist on the bridge model so a typo or rename surfaces at import, not on the next operator hot-reload. diff --git a/src/synthorg/api/controllers/audit.py b/src/synthorg/api/controllers/audit.py index f48b64be64..5a4ae44fb9 100644 --- a/src/synthorg/api/controllers/audit.py +++ b/src/synthorg/api/controllers/audit.py @@ -41,13 +41,9 @@ # Module-level log-once guard for the settings-resolution fallback: # during a prolonged settings outage this endpoint is queried once per -# request and would otherwise flood the logs with identical warnings; -# the flag is set on first failure and cleared on the next successful -# resolution so a later outage is visible again. The activities -# controller has migrated this knob to ``app_state.api_bridge_config`` -# (startup snapshot + ``ApiBridgeSettingsSubscriber`` hot-reload); this -# controller follows the same pattern when its cap is similarly -# bridged. +# request and would otherwise flood the logs with identical warnings. +# The flag is set on first failure and cleared on the next successful +# resolution so a later outage is visible again. _audit_cap_fallback_logged: bool = False diff --git a/src/synthorg/api/controllers/coordination_metrics.py b/src/synthorg/api/controllers/coordination_metrics.py index 62e86ba220..bb2d8460eb 100644 --- a/src/synthorg/api/controllers/coordination_metrics.py +++ b/src/synthorg/api/controllers/coordination_metrics.py @@ -36,13 +36,9 @@ # Module-level log-once guard for the settings-resolution fallback: # during a prolonged settings outage this endpoint is queried once per -# request and would otherwise flood the logs with identical warnings; -# the flag is set on first failure and cleared on the next successful -# resolution so a later outage is visible again. The activities -# controller has migrated this knob to ``app_state.api_bridge_config`` -# (startup snapshot + ``ApiBridgeSettingsSubscriber`` hot-reload); this -# controller follows the same pattern when its cap is similarly -# bridged. +# request and would otherwise flood the logs with identical warnings. +# The flag is set on first failure and cleared on the next successful +# resolution so a later outage is visible again. _metrics_cap_fallback_logged: bool = False diff --git a/src/synthorg/settings/subscribers/api_bridge_subscriber.py b/src/synthorg/settings/subscribers/api_bridge_subscriber.py index 17e21547be..7523e49f1a 100644 --- a/src/synthorg/settings/subscribers/api_bridge_subscriber.py +++ b/src/synthorg/settings/subscribers/api_bridge_subscriber.py @@ -7,11 +7,11 @@ invoking subscribers, so the watched set here only enumerates the hot-reloadable fields. -Currently watches ``api.max_lifecycle_events_per_query``, consumed by +Watches ``api.max_lifecycle_events_per_query``, consumed by :class:`~synthorg.api.controllers.activities.ActivityController` as -its ``LIMIT`` clamp on the lifecycle-events query. Other -``ApiBridgeConfig`` fields can be added to ``_WATCHED`` as their -controllers migrate off per-request fallback constants. +its ``LIMIT`` clamp on the lifecycle-events query. Additional +``ApiBridgeConfig`` fields can be appended to ``_WATCHED`` when their +consumers need hot-reload semantics. """ from typing import TYPE_CHECKING @@ -62,12 +62,14 @@ class ApiBridgeSettingsSubscriber: every other consumer does). On a watched-key change the subscriber resolves the integer value - via :class:`~synthorg.settings.resolver.ConfigResolver`, builds the - next snapshot through ``model_copy(update=...)`` (preserves every - other field), and hands it to ``AppState.swap_api_bridge_config``. - Resolver failures are logged via ``SETTINGS_SERVICE_SWAP_FAILED`` - and re-raised so the dispatcher records subscriber context; the - previous snapshot stays in place. + via :class:`~synthorg.settings.resolver.ConfigResolver` and applies + it through ``AppState.mutate_api_bridge_config({key: value})``, + which merges the single-field update into the current snapshot + under a per-bridge lock and re-validates via ``model_validate`` + (Pydantic v2 skips validators on the bare ``model_copy(update=...)`` + path). Resolver failures and validation errors are logged via + ``SETTINGS_SERVICE_SWAP_FAILED`` and re-raised so the dispatcher + records subscriber context; the previous snapshot stays in place. Args: app_state: Application state that owns the live snapshot. From abf2ceac4cfa3b5eb539a6ba908afb653317f514 Mon Sep 17 00:00:00 2001 From: Aurelio <19254254+Aureliolo@users.noreply.github.com> Date: Sat, 9 May 2026 15:07:01 +0200 Subject: [PATCH 5/5] fix: shift no-magic-numbers baseline pins for audit/metrics controllers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The migration-framing comment trim in the prior commit removed 4 lines from each of audit.py and coordination_metrics.py preambles, which shifted the pinned limit=50 default-arg sites from line 108→104 and 100→96. Update the baseline coordinates to match; no new magic-number sites introduced. --- scripts/no_magic_numbers_baseline.txt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/no_magic_numbers_baseline.txt b/scripts/no_magic_numbers_baseline.txt index e99ff32a03..2c9a01a1b0 100644 --- a/scripts/no_magic_numbers_baseline.txt +++ b/scripts/no_magic_numbers_baseline.txt @@ -44,7 +44,7 @@ src/synthorg/api/controllers/approvals.py:75:40 src/synthorg/api/controllers/approvals.py:519:29 src/synthorg/api/controllers/artifacts.py:181:29 src/synthorg/api/controllers/audit.py:39:19 -src/synthorg/api/controllers/audit.py:108:29 +src/synthorg/api/controllers/audit.py:104:29 src/synthorg/api/controllers/backup.py:187:29 src/synthorg/api/controllers/budget.py:302:29 src/synthorg/api/controllers/budget_config_versions.py:46:29 @@ -54,7 +54,7 @@ src/synthorg/api/controllers/connections.py:65:20 src/synthorg/api/controllers/connections.py:66:22 src/synthorg/api/controllers/connections.py:156:29 src/synthorg/api/controllers/coordination_metrics.py:34:21 -src/synthorg/api/controllers/coordination_metrics.py:100:29 +src/synthorg/api/controllers/coordination_metrics.py:96:29 src/synthorg/api/controllers/custom_rules.py:205:29 src/synthorg/api/controllers/custom_rules.py:435:29 src/synthorg/api/controllers/departments.py:229:49