perf(config): stop deep-copying the global config on every resolution (#4209) (#4211)

ConfigResolver converted the immutable global config with dataclasses.asdict() on every resolution, and config resolution is per-request, so a 400+ field deep walk landed on the hot path of every recall and every retain sub-batch. The conversion was only ever used as a shallow copy: the dict was spliced with the tenant/bank overrides and fed straight back into HindsightConfig(**dict). Resolve with copy.copy + setattr instead. That also drops the nine-field replace() that existed purely to undo asdict()'s recursion -- it flattened the nested member dataclasses (llm_members and friends) into plain dicts, which the old code then had to restore by hand. The two read paths get the same treatment: get_bank_config now projects the ~50 API-visible fields straight off the resolved object instead of materializing all 400+ and discarding most, and get_bank_configs builds its base from that same projection. Container values are copied on the way out, so a caller editing a returned list or dict still cannot reach the process-global config -- the property asdict()'s deep copy used to provide for free. Measured on the default config, per call: resolve_full_config 374.2us -> 3.5us (107x) get_bank_config 629.3us -> 10.9us (58x) get_bank_configs(x1) 254.4us -> 8.2us (31x) get_bank_configs(x50) 859.6us -> 38.6us (22x) Over 200 recall-shaped resolutions the Python-level call count drops from 3,856,401 to 42,801 (90x), with 256,800 deepcopy calls and 1.4M isinstance calls gone entirely.
2026-09-14 19:31:49 +08:00 · 2026-09-08 11:36:52 +02:00
parent 5f9bff9056
commit 3e1d47fdc6
2 changed files with 282 additions and 61 deletions
@@ -9,9 +9,10 @@ multiple API servers.
 """

 import asyncio
+import copy
 import json
 import logging
-from dataclasses import asdict, dataclass, fields, replace
+from dataclasses import dataclass, fields, replace
 from functools import lru_cache
 from types import UnionType
 from typing import TYPE_CHECKING, Any, Union, get_args, get_origin
@@ -78,7 +79,21 @@ class ValidatedBankConfigUpdate:
    """

    updates: dict[str, Any]
-    parent_config: dict[str, Any] | None = None
+    parent_config: HindsightConfig | None = None
+
+
+def _detached(value: Any) -> Any:
+    """Copy container values on their way out of a config object.
+
+    Config reads used to go through ``asdict()``, which deep-copied everything, so a
+    caller could edit the dict it got back. Reading fields straight off the object is
+    ~100x cheaper but hands out the process-global config's own lists and dicts, so
+    copy those few (``retain_strategies``, ``memory_defense``, ``entity_labels``, ...).
+    Scalars and strings are immutable and pass through untouched.
+    """
+    if isinstance(value, dict | list):
+        return copy.deepcopy(value)
+    return value


 def _validate_retain_strategy_chunking(base_config: HindsightConfig, strategies: Any) -> None:
@@ -114,7 +129,7 @@ def _validate_retain_strategy_chunking(base_config: HindsightConfig, strategies:


 def _validate_projected_bank_config(
-    parent_config: dict[str, Any],
+    parent_config: HindsightConfig,
    current_overrides: dict[str, Any],
    normalized_updates: dict[str, Any],
    configurable_fields: set[str],
@@ -134,7 +149,7 @@ def _validate_projected_bank_config(
        else:
            projected[key] = value

-    base_config = HindsightConfig(**{**parent_config, **projected})
+    base_config = replace(parent_config, **projected) if projected else parent_config
    validate_retain_chunking_config(
        base_config.retain_chunk_size,
        base_config.retain_structured_chunk_size,
@@ -162,26 +177,51 @@ class ConfigResolver:
        self._global_config = _get_raw_config()
        self._configurable_fields = HindsightConfig.get_configurable_fields()
        self._credential_fields = HindsightConfig.get_credential_fields()
+        # The API-visible subset, resolved once: every config read filters to it.
+        self._public_fields = frozenset(self._configurable_fields - self._credential_fields)

-    async def _resolve_parent_config_dict(self, bank_id: str, context: RequestContext | None = None) -> dict[str, Any]:
-        """Resolve global + tenant config before bank-level overrides."""
-        config_dict = asdict(self._global_config)
+    async def _resolve_tenant_overrides(self, scope: str, context: RequestContext | None = None) -> dict[str, Any]:
+        """Tenant-level overrides to apply on top of the global config.

-        if self.tenant_extension and context:
-            try:
-                tenant_overrides = await self.tenant_extension.get_tenant_config(context)
-                if tenant_overrides:
-                    # Normalize keys and filter to configurable fields only
-                    normalized_tenant = normalize_config_dict(tenant_overrides)
-                    configurable_tenant = {k: v for k, v in normalized_tenant.items() if k in self._configurable_fields}
-                    config_dict.update(configurable_tenant)
-                    logger.debug(
-                        f"Applied tenant config overrides for bank {bank_id}: {list(configurable_tenant.keys())}"
-                    )
-            except Exception as e:
-                logger.warning(f"Failed to load tenant config for bank {bank_id}: {e}")
+        Returns only the fields the tenant actually overrides — the global config is
+        the base *object*, not a dict we rebuild per request. ``scope`` names the
+        caller (a bank id, or a description of a bulk resolve) for log messages only;
+        tenant config is per-request, not per-bank.
+        """
+        if not (self.tenant_extension and context):
+            return {}
+        try:
+            tenant_overrides = await self.tenant_extension.get_tenant_config(context)
+        except Exception as e:
+            logger.warning(f"Failed to load tenant config for {scope}: {e}")
+            return {}
+        if not tenant_overrides:
+            return {}
+        # Normalize keys and filter to configurable fields only
+        normalized_tenant = normalize_config_dict(tenant_overrides)
+        configurable_tenant = {k: v for k, v in normalized_tenant.items() if k in self._configurable_fields}
+        if configurable_tenant:
+            logger.debug(f"Applied tenant config overrides for {scope}: {list(configurable_tenant.keys())}")
+        return configurable_tenant

-        return config_dict
+    def _with_overrides(self, overrides: dict[str, Any]) -> HindsightConfig:
+        """Global config with ``overrides`` applied, as a fresh object.
+
+        ``copy.copy`` rather than ``asdict()`` + ``HindsightConfig(**dict)``: the config
+        has 400+ fields and resolution is per-request, so deep-walking the whole tree to
+        produce what is only ever used as a shallow copy dominated the recall hot path
+        (see #4209). A shallow copy is also *more* correct — ``asdict()`` flattened the
+        nested member dataclasses (``llm_members`` and friends) into plain dicts, which
+        the old code then had to restore field by field.
+
+        Safe because ``HindsightConfig`` is a plain non-frozen dataclass: no
+        ``__post_init__``, no ``init=False`` fields, no ``__slots__``, and no non-field
+        instance attributes. A test pins those properties.
+        """
+        resolved = copy.copy(self._global_config)
+        for key, value in overrides.items():
+            setattr(resolved, key, value)
+        return resolved

    async def resolve_full_config(
        self, bank_id: str, context: RequestContext | None = None, *, cached: bool = True
@@ -204,33 +244,15 @@ class ConfigResolver:
        Returns:
            Complete HindsightConfig with hierarchical overrides applied
        """
-        config_dict = await self._resolve_parent_config_dict(bank_id, context)
+        overrides = await self._resolve_tenant_overrides(f"bank {bank_id}", context)

        # Load bank config overrides
        bank_overrides = await self._load_bank_config(bank_id, cached=cached)
        if bank_overrides:
-            config_dict.update(bank_overrides)
+            overrides.update(bank_overrides)
            logger.debug(f"Applied bank config overrides for bank {bank_id}: {list(bank_overrides.keys())}")

-        # Return full config object (dataclass doesn't have __init__ that accepts kwargs, so we update the object)
-        # Create a new config instance by copying the global config and updating fields
-        resolved_config = HindsightConfig(**config_dict)
-        # Multi-LLM chains and the reranker failover chain are static credential fields
-        # (never tenant/bank-overridable), but asdict() above flattened their member
-        # dataclasses into plain dicts. Restore the original typed objects from the global
-        # config so the resolved object stays well-typed for any consumer that reads them.
-        resolved_config = replace(
-            resolved_config,
-            reranker_members=self._global_config.reranker_members,
-            llm_members=self._global_config.llm_members,
-            llm_strategy=self._global_config.llm_strategy,
-            retain_llm_members=self._global_config.retain_llm_members,
-            retain_llm_strategy=self._global_config.retain_llm_strategy,
-            reflect_llm_members=self._global_config.reflect_llm_members,
-            reflect_llm_strategy=self._global_config.reflect_llm_strategy,
-            consolidation_llm_members=self._global_config.consolidation_llm_members,
-            consolidation_llm_strategy=self._global_config.consolidation_llm_strategy,
-        )
+        resolved_config = self._with_overrides(overrides)
        validate_retain_chunking_config(
            resolved_config.retain_chunk_size,
            resolved_config.retain_structured_chunk_size,
@@ -275,21 +297,29 @@ class ConfigResolver:
        """
        # Resolve full config with all hierarchical overrides
        resolved_config = await self.resolve_full_config(bank_id, context, cached=cached)
-        config_dict = asdict(resolved_config)

        # SECURITY: drop static/infrastructure + credential fields, then permission-filter.
-        filtered = self._strip_static_and_credential_fields(config_dict)
+        filtered = self._public_fields_of(resolved_config)
        return await self._apply_permission_filter(filtered, bank_id, context)

-    def _strip_static_and_credential_fields(self, config_dict: dict[str, Any]) -> dict[str, Any]:
-        """Keep only configurable, non-credential fields.
+    def _public_fields_of(self, config: HindsightConfig) -> dict[str, Any]:
+        """Project a resolved config down to its configurable, non-credential fields.
+
+        Reads the ~50 public fields straight off the object instead of materializing
+        all 400+ with ``asdict()`` and throwing most away (#4209).

        SECURITY: excludes static/infrastructure fields and ALL credential fields
        (API keys, base URLs, etc.) so a resolved config is safe to return over the API.
        """
-        return {
-            k: v for k, v in config_dict.items() if k in self._configurable_fields and k not in self._credential_fields
-        }
+        return {name: _detached(getattr(config, name)) for name in self._public_fields}
+
+    def _strip_static_and_credential_fields(self, config_dict: dict[str, Any]) -> dict[str, Any]:
+        """Keep only configurable, non-credential fields of an already-dict config.
+
+        SECURITY: same allow-list as :meth:`_public_fields_of`, for the paths that
+        merge plain override dicts rather than resolved config objects.
+        """
+        return {k: v for k, v in config_dict.items() if k in self._public_fields}

    async def _apply_permission_filter(
        self, filtered: dict[str, Any], bank_id: str, context: RequestContext | None
@@ -330,20 +360,17 @@ class ConfigResolver:
            return {}

        # Global + tenant base, resolved once (tenant override is per-request, not per-bank).
-        base_dict = asdict(self._global_config)
-        if self.tenant_extension and context:
-            try:
-                tenant_overrides = await self.tenant_extension.get_tenant_config(context)
-                if tenant_overrides:
-                    normalized_tenant = normalize_config_dict(tenant_overrides)
-                    base_dict.update({k: v for k, v in normalized_tenant.items() if k in self._configurable_fields})
-            except Exception as e:
-                logger.warning(f"Failed to load tenant config for bulk resolve: {e}")
+        base_dict = self._public_fields_of(self._global_config)
+        tenant_base = await self._resolve_tenant_overrides("bulk resolve", context)
+        base_dict.update(self._strip_static_and_credential_fields(tenant_base))

        # All bank overrides in one query, then merge + strip per bank.
        bank_overrides = await self._load_bank_configs(bank_ids)
        stripped = {
-            bank_id: self._strip_static_and_credential_fields({**base_dict, **bank_overrides.get(bank_id, {})})
+            bank_id: {
+                **base_dict,
+                **self._strip_static_and_credential_fields(bank_overrides.get(bank_id, {})),
+            }
            for bank_id in bank_ids
        }

@@ -580,9 +607,9 @@ class ConfigResolver:
        # bad update is rejected before the bank is created; _persist_bank_config
        # repeats it against the committed state under the bank row lock, which
        # is what makes the result independent of request interleaving.
-        parent_config: dict[str, Any] | None = None
+        parent_config: HindsightConfig | None = None
        if not _CROSS_FIELD_CONSTRAINED_FIELDS.isdisjoint(normalized_updates):
-            parent_config = await self._resolve_parent_config_dict(bank_id, context)
+            parent_config = self._with_overrides(await self._resolve_tenant_overrides(f"bank {bank_id}", context))
            current_overrides = (
                await self._load_bank_config(bank_id)
                if projected_bank_overrides is None
@@ -0,0 +1,194 @@
+"""Shape and cost guarantees for ConfigResolver's per-request resolution.
+
+Resolution runs on every recall and every retain sub-batch, so how it builds the
+resolved config matters as much as what it resolves. These tests pin the
+properties the cheap path depends on (#4209): the config is shallow-copyable,
+the copy is indistinguishable from the dict round-trip it replaced, typed member
+dataclasses survive it, and a caller still cannot reach into the process-global
+config through a value it was handed.
+"""
+
+import copy
+import dataclasses
+from dataclasses import asdict, replace
+
+import pytest
+
+from hindsight_api.config import HindsightConfig, LLMMemberConfig, LLMStrategyConfig, _get_raw_config
+from hindsight_api.config_resolver import ConfigResolver
+
+from .test_hierarchical_config import FakeBankConfigBackend, FakeBankConfigConnection, MockTenantExtension
+
+BANK = "test-config-shape-bank"
+
+
+class _BulkCapableBackend(FakeBankConfigBackend):
+    """FakeBankConfigBackend plus the ``fetch`` the bulk path needs."""
+
+    def acquire(self):
+        return _BulkCapableConnection(self)
+
+    def transaction(self):
+        return _BulkCapableConnection(self)
+
+
+class _BulkCapableConnection(FakeBankConfigConnection):
+    async def fetch(self, query, bank_ids):
+        return [{"bank_id": bank_id, "config": self.backend.config} for bank_id in bank_ids]
+
+
+def _resolver(**kwargs) -> ConfigResolver:
+    return ConfigResolver(backend=FakeBankConfigBackend(), **kwargs)
+
+
+def test_hindsight_config_is_safely_shallow_copyable():
+    """Pin the dataclass properties ``_with_overrides`` relies on.
+
+    ``copy.copy`` is only interchangeable with a full reconstruction while the
+    config has no ``__post_init__``, no ``init=False`` fields, no ``__slots__``,
+    and no instance state outside its fields. Adding any of those to
+    HindsightConfig would silently change what resolution returns, so fail here
+    rather than in production.
+    """
+    config = _get_raw_config()
+
+    assert not hasattr(config, "__post_init__"), "a __post_init__ would be skipped by copy.copy"
+    assert not hasattr(HindsightConfig, "__slots__"), "__slots__ would break vars()-based copying"
+    assert [f.name for f in dataclasses.fields(config) if not f.init] == [], (
+        "an init=False field cannot be round-tripped through the constructor"
+    )
+    assert set(vars(config)) == {f.name for f in dataclasses.fields(config)}, (
+        "non-field instance attributes would not survive a reconstruction"
+    )
+    assert copy.copy(config) == replace(config)
+
+
+@pytest.mark.asyncio
+async def test_resolved_config_matches_dict_roundtrip():
+    """The copy-based resolution returns exactly what the asdict() one did.
+
+    The reference below is the pre-#4209 construction: flatten the global config
+    with asdict(), splice the overrides in, rebuild, then restore the member
+    dataclasses asdict() had turned into plain dicts.
+    """
+    tenant = MockTenantExtension({"retain_extraction_mode": "tenant-mode", "retain_chunk_size": 5000})
+    resolver = ConfigResolver(backend=FakeBankConfigBackend(), tenant_extension=tenant)
+    global_config = resolver._global_config
+    context = _context()
+
+    for overrides in ({}, {"retain_chunk_size": 2000, "enable_observations": False}):
+        resolver._backend.config = dict(overrides)
+
+        resolved = await resolver.resolve_full_config(BANK, context, cached=False)
+
+        merged = asdict(global_config)
+        merged.update({"retain_extraction_mode": "tenant-mode", "retain_chunk_size": 5000})
+        merged.update(overrides)
+        expected = replace(
+            HindsightConfig(**merged),
+            reranker_members=global_config.reranker_members,
+            llm_members=global_config.llm_members,
+            llm_strategy=global_config.llm_strategy,
+            retain_llm_members=global_config.retain_llm_members,
+            retain_llm_strategy=global_config.retain_llm_strategy,
+            reflect_llm_members=global_config.reflect_llm_members,
+            reflect_llm_strategy=global_config.reflect_llm_strategy,
+            consolidation_llm_members=global_config.consolidation_llm_members,
+            consolidation_llm_strategy=global_config.consolidation_llm_strategy,
+        )
+        assert resolved == expected
+        assert resolved is not global_config, "resolution must not hand out the process-global config"
+
+
+@pytest.mark.asyncio
+async def test_resolution_preserves_typed_member_dataclasses():
+    """Multi-LLM members stay dataclasses, not the dicts asdict() flattened them into."""
+    resolver = _resolver()
+    member = LLMMemberConfig(
+        provider="openai",
+        api_key="k",
+        model="gpt-4",
+        base_url=None,
+        reasoning_effort=None,
+        extra_body=None,
+        default_headers=None,
+        bedrock_service_tier=None,
+        gemini_service_tier=None,
+    )
+    strategy = LLMStrategyConfig(mode="failover")
+    resolver._global_config = replace(resolver._global_config, llm_members=[member], llm_strategy=strategy)
+
+    resolved = await resolver.resolve_full_config(BANK, cached=False)
+
+    assert resolved.llm_members == [member]
+    assert isinstance(resolved.llm_members[0], LLMMemberConfig)
+    assert isinstance(resolved.llm_strategy, LLMStrategyConfig)
+
+
+@pytest.mark.asyncio
+async def test_resolution_does_not_mutate_the_global_config():
+    """Overrides land on the copy, never on the shared global object."""
+    resolver = _resolver()
+    before = replace(resolver._global_config)
+    resolver._backend.config = {"retain_chunk_size": before.retain_chunk_size + 111}
+
+    resolved = await resolver.resolve_full_config(BANK, cached=False)
+
+    assert resolved.retain_chunk_size == before.retain_chunk_size + 111
+    assert resolver._global_config == before
+
+
+@pytest.mark.asyncio
+async def test_bank_config_response_containers_are_detached():
+    """A caller editing a returned container must not reach the global config.
+
+    Config reads used to go through asdict(), which deep-copied every value.
+    Reading fields off the object is far cheaper but would otherwise hand out the
+    process-global config's own dicts and lists.
+    """
+    resolver = _resolver()
+    resolver._global_config = replace(resolver._global_config, retain_strategies={"fast": {"retain_chunk_size": 900}})
+
+    config = await resolver.get_bank_config(BANK, cached=False)
+    config["retain_strategies"]["fast"]["retain_chunk_size"] = 1
+    config["retain_strategies"]["injected"] = {}
+
+    assert resolver._global_config.retain_strategies == {"fast": {"retain_chunk_size": 900}}
+    second = await resolver.get_bank_config(BANK, cached=False)
+    assert second["retain_strategies"] == {"fast": {"retain_chunk_size": 900}}
+
+
+@pytest.mark.asyncio
+async def test_bank_config_excludes_static_and_credential_fields():
+    """The public projection is exactly configurable-minus-credential fields."""
+    resolver = _resolver()
+
+    config = await resolver.get_bank_config(BANK, cached=False)
+
+    configurable = HindsightConfig.get_configurable_fields()
+    credentials = HindsightConfig.get_credential_fields()
+    assert set(config) == configurable - credentials
+    assert not set(config) & credentials
+    assert "database_url" not in config
+
+
+@pytest.mark.asyncio
+async def test_bulk_and_single_bank_config_agree():
+    """get_bank_configs is the batched form of get_bank_config, including tenant overrides."""
+    tenant = MockTenantExtension({"retain_chunk_size": 5000})
+    resolver = ConfigResolver(backend=_BulkCapableBackend(), tenant_extension=tenant)
+    resolver._backend.config = {"enable_observations": False}
+    context = _context()
+
+    single = await resolver.get_bank_config(BANK, context, cached=False)
+    bulk = await resolver.get_bank_configs([BANK], context)
+
+    assert bulk[BANK] == single
+    assert single["retain_chunk_size"] == 5000
+    assert single["enable_observations"] is False
+
+
+def _context():
+    from hindsight_api.models import RequestContext
+
+    return RequestContext(api_key=None, api_key_id=None, tenant_id=None, internal=False)