diff --git a/docs/reference/configuration-precedence.md b/docs/reference/configuration-precedence.md index 4ce2b7922b..5ef9ae57a0 100644 --- a/docs/reference/configuration-precedence.md +++ b/docs/reference/configuration-precedence.md @@ -141,6 +141,54 @@ 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; `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. + +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/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..5a4ae44fb9 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..bb2d8460eb 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..cc3571c572 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,40 @@ 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 asyncio.CancelledError: + raise + 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 +809,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..6f6156b145 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,8 @@ class AppState(AppStateServicesMixin): "_agent_registry", "_agent_version_service", "_analytics_service", + "_api_bridge_config", + "_api_bridge_config_lock", "_approval_gate", "_approval_timeout_scheduler", "_artifact_facade_service", @@ -415,6 +418,21 @@ 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). 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 1fd7f91af0..dc4bcea551 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,8 @@ 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 @@ -1040,6 +1043,81 @@ 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 :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 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. + """ + 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", + transition="mutate", + changed_fields=sorted(updates), + ) + @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..7523e49f1a --- /dev/null +++ b/src/synthorg/settings/subscribers/api_bridge_subscriber.py @@ -0,0 +1,127 @@ +"""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. + +Watches ``api.max_lifecycle_events_per_query``, consumed by +:class:`~synthorg.api.controllers.activities.ActivityController` as +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 + +from synthorg.observability import get_logger, safe_error_description +from synthorg.observability.events.settings import ( + SETTINGS_SERVICE_SWAP_FAILED, + SETTINGS_SUBSCRIBER_NOTIFIED, +) +from synthorg.settings.bridge_configs import ApiBridgeConfig + +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"), + } +) + +# 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. + + 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` 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. + 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) + self._app_state.mutate_api_bridge_config({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 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..0da545a964 --- /dev/null +++ b/tests/unit/settings/test_api_bridge_subscriber.py @@ -0,0 +1,171 @@ +"""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 + + 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.""" + + 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